[Kernel] [CatalogManaged] Add new builder .withCommitter API; Create DefaultCommitter skeleton #4936
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
scottsand-db
changed the title
scottsand-db
requested review from
nicklan,
raveeram-db,
zachschuermann and
huan233usc
scottsand-db
commented
Jul 15, 2025
| import io.delta.kernel.internal.tablefeatures.TableFeatures; | ||
| import io.delta.kernel.utils.CloseableIterator; | ||
|
|
||
| public class DefaultFileSystemManagedTableOnlyCommitter implements Committer { |
There was a problem hiding this comment.
note: This will get more complex later on as we implement it properly
huan233usc
reviewed
Jul 16, 2025
| throw new UnsupportedOperationException("Default Committer not yet implemented"); | ||
| } | ||
|
|
||
| private void validateProtocol(Protocol protocol) { |
There was a problem hiding this comment.
Is it intentionally included in this PR? it looks a bit wired that we do some check then throw unsupported error. (but ok with that given DefaultFileSystemManagedTableOnlyCommitter will be implemented shortly)
There was a problem hiding this comment.
Good question! Yup, intentionally included.
I think it's important for us to all agree from the get go that the default committer should not be able to write into catalog managed tables.
emkornfield
approved these changes
Jul 16, 2025
huan233usc
approved these changes
Jul 24, 2025
scottsand-db
force-pushed
the
stack/kernel_catalog_managed_writes_get_committer
branch
from
0d0d475 to
9e7062d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🥞 Stacked PR
Use this link to review incremental changes.
Which Delta project/connector is this regarding?
Description
#4814 introduced new TransactionV2, Committer, CommitContext, etc APIs.
This PR now adds the abilities for Engines to inject into Kernel the actual Committer.
This PR also includes a no-op Default committer implementation that validates that it is not interacting with catalog-managed tables. We do this because: ResolvedTable does not return an Optional committer, but rather just a committer. So we need to give it something in the case that no committer was provided to the table builder.
How was this patch tested?
New UTs.
Does this PR introduce any user-facing changes?
New .withCommitter API