-
Notifications
You must be signed in to change notification settings - Fork 2k
[PROTOTYPE] [EARLY FEEDBACK REQUESTED] Kernel accept opaque committer properties; UC committer extensibility #5230
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,7 @@ | |
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Optional; | ||
| import java.util.function.Supplier; | ||
|
|
||
| /** | ||
| * Represents a transaction to mutate a Delta table. | ||
|
|
@@ -108,6 +109,12 @@ public interface Transaction { | |
| TransactionCommitResult commit(Engine engine, CloseableIterable<Row> dataActions) | ||
| throws ConcurrentWriteException; | ||
|
|
||
| /** | ||
| * Adds custom properties that will be passed through to the committer. These properties allow | ||
| * connectors to inject catalog-specific metadata without Kernel inspection. | ||
| */ | ||
| void withCommitterProperties(Supplier<Map<String, String>> committerProperties); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added here instead of on the builder so Connectors can inject any properies they'd like at any point. e.g. maybe these properties depend on the data files written |
||
|
|
||
| /** | ||
| * Commit the provided domain metadata as part of this transaction. If this is called more than | ||
| * once with the same {@code domain} the latest provided {@code config} will be committed in the | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| import io.delta.kernel.SnapshotBuilder; | ||
| import io.delta.kernel.TableManager; | ||
| import io.delta.kernel.annotation.Experimental; | ||
| import io.delta.kernel.commit.Committer; | ||
| import io.delta.kernel.engine.Engine; | ||
| import io.delta.kernel.internal.annotation.VisibleForTesting; | ||
| import io.delta.kernel.internal.files.ParsedLogData; | ||
|
|
@@ -56,14 +57,14 @@ public class UCCatalogManagedClient { | |
| * add this table feature to Kernel's protocol. This property won't be written to the delta | ||
| * metadata. | ||
| */ | ||
| private static final String CATALOG_MANAGED_ENABLEMENT_KEY = | ||
| protected static final String CATALOG_MANAGED_ENABLEMENT_KEY = | ||
| TableFeatures.SET_TABLE_FEATURE_SUPPORTED_PREFIX | ||
| + TableFeatures.CATALOG_MANAGED_R_W_FEATURE_PREVIEW.featureName(); | ||
|
|
||
| /** Key for identifying Unity Catalog table ID. */ | ||
| private static final String UC_TABLE_ID_KEY = "ucTableId"; | ||
| protected static final String UC_TABLE_ID_KEY = "ucTableId"; | ||
|
|
||
| private final UCClient ucClient; | ||
| protected final UCClient ucClient; | ||
|
|
||
| public UCCatalogManagedClient(UCClient ucClient) { | ||
| this.ucClient = Objects.requireNonNull(ucClient, "ucClient is null"); | ||
|
|
@@ -112,7 +113,7 @@ public Snapshot loadSnapshot( | |
| } | ||
|
|
||
| return snapshotBuilder | ||
| .withCommitter(new UCCatalogManagedCommitter(ucClient, ucTableId, tablePath)) | ||
| .withCommitter(createUCCommitter(ucClient, ucTableId, tablePath)) | ||
| .withLogData(logData) | ||
| .build(engine); | ||
| }); | ||
|
|
@@ -142,10 +143,24 @@ public CreateTableTransactionBuilder buildCreateTableTransaction( | |
| Objects.requireNonNull(engineInfo, "engineInfo is null"); | ||
|
|
||
| return TableManager.buildCreateTableTransaction(tablePath, schema, engineInfo) | ||
| .withCommitter(new UCCatalogManagedCommitter(ucClient, ucTableId, tablePath)) | ||
| .withCommitter(createUCCommitter(ucClient, ucTableId, tablePath)) | ||
| .withTableProperties(getRequiredTablePropertiesForCreate(ucTableId)); | ||
| } | ||
|
|
||
| ///////////////////////////////////////// | ||
| // Protected Methods for Extensibility // | ||
| ///////////////////////////////////////// | ||
|
|
||
| /** | ||
| * Creates a UC committer instance for the specified table. | ||
| * | ||
| * <p>This method allows subclasses to provide custom committer implementations for specialized | ||
| * use cases. | ||
| */ | ||
| protected Committer createUCCommitter(UCClient ucClient, String ucTableId, String tablePath) { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Code smell here. I wonder if we should abandon the UCCatalogManagedClient::loadSnapshot API -- and instead just provide static utilities (like getting the commits, parsing the commits, etc.) |
||
| return new UCCatalogManagedCommitter(ucClient, ucTableId, tablePath); | ||
| } | ||
|
|
||
| //////////////////// | ||
| // Helper Methods // | ||
| //////////////////// | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love how this is on the Txn and not the TxnBuilder ...