Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: introduce LogStore trait #1706

Closed
wants to merge 7 commits into from

Conversation

dispanser
Copy link
Contributor

This trait is supposed to serve as the entry point to read and write commits in the Delta log.

Description

This PR introduces a new trait, LogStore, which is meant to encapsulate interaction with the delta commit log, i.e. things in the directory _delta_log/. This is half a PR and half a discussion starting point, hence the PR description that is substantially longer than the actual proposed code change :-).

The major goal of this exercise is to align the implementation of multi-cluster writes for Delta Lake on S3 with the one provided by the original delta library, enabling multi-cluster writes with some writers using Spark / Delta library and other writers using delta-rs For an overview of how it's done in delta, please see:

  1. Delta blog post (high-level concept)
  2. Associated Databricks design doc (detailed read)
  3. S3DynamoDbLogStore.java(content warning: Java code behind this link)

This approach requires readers of a delta table to "recover" unfinished commits from writers - as a result, reading and writing is combined in a single interface, which in this PR is modeled after LogStore.java. Currently in delta-rs, read path for commits is implemented directly in DeltaTable, and there's no mechanism to implement storage-specific behavior like interacting with DynamoDb.

In this draft, LogStore provides:

  • read_commit_entry(version) to read a commit entry representing a specific version
  • write_commit_entry(version, actions) to write a set of actions as a commit entry
  • get_latest_version() to find the latest version in the delta log

This trait could be extended to cover all interactions with anything inside _delta_log, e.g. checkpoints, finding latest commit for a timestamp, etc. However, this is not necessary to implement the S3 log store, and additional functionality can be integrated incrementally when it makes sense.

Implementation

This PR does not include an actual implementation for LogStore or its potential integration into the existing code base. It serves as a basis for a discussion and feedback on the proposed changes only.

However, some thoughts on how an implementation would fit in:
A default implementation of trait LogStore would be configured with a location and an ObjectStore (or DeltaObjectStore), and combine functionality that is currently distributed over src/table/mod.rs (reading data for individual commits) and src/writer/mod.rs (try_commit_transaction mostly). It would probably be owned by DeltaTable.
The S3 + dynamo implementation would be different, utilizing DynamoDb for both read and write operations to enable multi-cluster writes on S3. I believe all other object stores are able to share a single implementation for now, as they sort of do in the current code base anyways.

@github-actions github-actions bot added binding/rust Issues for the Rust crate rust labels Oct 6, 2023
@github-actions
Copy link

github-actions bot commented Oct 6, 2023

ACTION NEEDED

delta-rs follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@rtyler rtyler marked this pull request as draft October 6, 2023 19:23
This trait is supposed to serve as the entry point to read and write
commits in the Delta log.
@dispanser dispanser changed the title Introduce LogStore trait refactor: introduce LogStore trait Oct 7, 2023
rtyler
rtyler previously approved these changes Oct 9, 2023
Copy link
Member

@rtyler rtyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my perspective I think we will benefit from having this abstraction, and it does line up with some other refactorings and improvement I am hoping to make, e.g. #1713

@rtyler rtyler marked this pull request as ready for review October 9, 2023 22:58
Copy link
Collaborator

@wjones127 wjones127 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of getting us in line with Databricks' DynamoDB protocol. Had one question on this interface.

&self,
version: i64,
actions: Vec<Action>,
overwrite: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does overwrite do? Why is it necessary?(I would think overwriting a commit would be not compatible with consistency.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point. I copied that over from LogStore::write from the reference implementation, but couldn't trace back any justification for the overwrite flag. The design doc linked above has the following to say:

If overwrite=true, then write directly into S3 with no DynamoDB interaction
else

which doesn't help me understand the "why" either. As you correctly point out, overwrite is violating any of the consistency the delta log is supposed to deliver in the first place.

If you don't see a use case / call to that method that would set overwrite = true, my proposal would be to drop that argument from write_commit_entry and potentially add it when we can justify its existence.

@dispanser
Copy link
Contributor Author

closing in favor of #1742

@dispanser dispanser closed this Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/rust Issues for the Rust crate rust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants