You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@xtable.apache.org by "Akash3121 (via GitHub)" <gi...@apache.org> on 2026/09/21 02:22:04 UTC
[PR] Fix Delta Kernel commit tag mapping for source-to-target commit identifiers [incubator-xtable]
Akash3121 opened a new pull request, #942:
URL: https://github.com/apache/incubator-xtable/pull/942
This PR restores source-to-target commit identifier tracking for the Delta Kernel path in support of issue #819.
Delta Kernel 4.0.0 does not expose the commit-tag flow used by the legacy Delta implementation, so XTable uses Delta Kernel domain metadata to persist the source commit identifier on the target commit and resolve it later from the latest transaction version.
This PR also restores Delta nested schema metadata propagation, Glue Delta storage-descriptor fields, and the Parquet limitations documentation that accompany the Delta Kernel work.
### Notes to reviewer
- Delta Kernel commit mapping now uses domain metadata, not commit tags.
- The schema extractor changes preserve column-mapping IDs, physical names, and nested IDs for arrays and maps.
- The Glue and docs updates are supporting fixes that came out of review, not the primary feature.
- Targeted unit tests for the touched modules pass; the broader `TestDeltaKernelSync` suite still hits the known Windows Hadoop/native limitation in this environment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [819] Fix:Delta Kernel commit tag mapping for source-to-target commit identifiers [incubator-xtable]
Posted by "Akash3121 (via GitHub)" <gi...@apache.org>.
Akash3121 commented on code in PR #942:
URL: https://github.com/apache/incubator-xtable/pull/942#discussion_r4209066017
##########
xtable-core/src/main/java/org/apache/xtable/delta/DeltaSchemaExtractor.java:
##########
@@ -84,25 +84,19 @@ private static Integer nestedFieldId(Metadata nestedIds, String path) {
return (int) nestedIds.getLong(path);
}
- public InternalSchema toInternalSchema(StructType structType) {
- return toInternalSchema(structType, null, false, null, null, null, null);
+ private static Integer nestedFieldId(
+ Metadata nestedIds, String nestedIdPath, String parquetChildName, String internalChildName) {
+ Integer fieldId = nestedFieldId(nestedIds, childIdPath(nestedIdPath, parquetChildName));
+ if (fieldId != null || parquetChildName.equals(internalChildName)) {
+ return fieldId;
+ }
+ return nestedFieldId(nestedIds, childIdPath(nestedIdPath, internalChildName));
}
- private InternalSchema toInternalSchema(
- DataType dataType,
- String parentPath,
- boolean nullable,
- String comment,
- Metadata originalMetadata) {
- return toInternalSchema(dataType, parentPath, nullable, comment, originalMetadata, null, null);
+ public InternalSchema toInternalSchema(StructType structType) {
+ return toInternalSchema(structType, null, false, null, null, null, null);
}
- /**
- * @param nestedIds the {@code delta.columnMapping.nested.ids} metadata of the enclosing struct
- * field, or null when the field carries none
- * @param nestedIdPath the path of the current type within that metadata, starting at the
- * enclosing field's physical name
- */
private InternalSchema toInternalSchema(
Review Comment:
Good catch, this was not intentional - I restored it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [819] Fix:Delta Kernel commit tag mapping for source-to-target commit identifiers [incubator-xtable]
Posted by "Akash3121 (via GitHub)" <gi...@apache.org>.
Akash3121 commented on code in PR #942:
URL: https://github.com/apache/incubator-xtable/pull/942#discussion_r4211653135
##########
xtable-core/src/main/java/org/apache/xtable/kernel/DeltaKernelConversionTarget.java:
##########
@@ -317,24 +314,26 @@ public String getTableFormat() {
@Override
public Optional<String> getTargetCommitIdentifier(String sourceIdentifier) {
- // Delta Kernel 4.0.0 does not support commit tags in commitInfo, which are required for
- // source-to-target commit identifier mapping. This limitation is documented in:
- // https://github.com/delta-io/delta/issues/6167
- // XTable tracking issue: https://github.com/apache/incubator-xtable/issues/819
- //
- // Unlike DeltaConversionTarget (which uses Delta Standalone with commit tag support),
- // DeltaKernelConversionTarget cannot retrieve commit tags from Delta Kernel's API.
- // Rather than silently scanning all commits (O(n) performance cost) and always returning
- // empty, we explicitly throw an exception to indicate this feature is unsupported.
- //
- // When Delta Kernel adds commit tag support, this method can be reimplemented to:
- // 1. Scan commit history using tableImpl.getChanges(engine, 0, currentVersion, actionSet)
- // 2. Extract tags from CommitInfo.tags MapValue
- // 3. Parse XTABLE_METADATA from tags and match sourceIdentifier
- throw new NotSupportedException(
- "Source-to-target commit identifier mapping is not supported in DeltaKernelConversionTarget. "
- + "Delta Kernel 4.0.0 does not support commit tags in commitInfo. "
- + "See: https://github.com/delta-io/delta/issues/6167");
+ if (sourceIdentifier == null || sourceIdentifier.trim().isEmpty()) {
+ return Optional.empty();
+ }
+
+ String sourceIdentifierDomain = getSourceIdentifierDomainName(sourceIdentifier);
+ Table table = Table.forPath(engine, basePath);
+ Snapshot latestSnapshot;
+ try {
+ latestSnapshot = table.getLatestSnapshot(engine);
+ } catch (TableNotFoundException e) {
+ return Optional.empty();
+ }
+
+ return ((SnapshotImpl) latestSnapshot)
+ .getLatestTransactionVersion(engine, sourceIdentifierDomain)
Review Comment:
You are correct, Good Catch. I removed the mismatched DomainMetadata/SetTransaction approach and now scan Delta Kernel’s retained metadata history for the matching XTABLE_METADATA, returning the commit version where it was originally recorded. I also added coverage for current and historical mappings, carried-forward and malformed metadata, unknown IDs, and invalid inputs.
The updated tests and CI build pass pls review when you have time @vinishjail97. Thank you
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix Delta Kernel commit tag mapping for source-to-target commit identifiers (#819) [incubator-xtable]
Posted by "vinishjail97 (via GitHub)" <gi...@apache.org>.
vinishjail97 commented on code in PR #942:
URL: https://github.com/apache/incubator-xtable/pull/942#discussion_r4067603121
##########
xtable-aws/src/main/java/org/apache/xtable/glue/table/DeltaGlueCatalogTableBuilder.java:
##########
@@ -48,14 +48,6 @@
/** Delta specific table operations for Glue catalog sync */
public class DeltaGlueCatalogTableBuilder implements CatalogTableBuilder<TableInput, Table> {
-
- // Standard Hive-compatible Parquet classes (Delta's on-disk file format).
- // A generic Hive-Metastore-compatible reader (e.g. a UC Glue Federation
- // foreign catalog) validates every table's StorageDescriptor when listing a
- // database and throws InvalidObjectException for the whole database on the
- // first table with a null SerDe/format -- these must be set even though
- // Athena's own Delta reader (which keys off the table_type/
- // spark.sql.sources.provider parameters instead) tolerates them being null.
private static final String PARQUET_INPUT_FORMAT =
Review Comment:
This comment, and the blank-line moves in `TestDeltaGlueCatalogTableBuilder`, revert what landed in 35eecfa5 (#940) a few days ago. Can we rebase with latest main? I think these were accidentally deleted.
##########
xtable-core/src/main/java/org/apache/xtable/kernel/DeltaKernelConversionTarget.java:
##########
@@ -317,24 +314,26 @@ public String getTableFormat() {
@Override
public Optional<String> getTargetCommitIdentifier(String sourceIdentifier) {
- // Delta Kernel 4.0.0 does not support commit tags in commitInfo, which are required for
- // source-to-target commit identifier mapping. This limitation is documented in:
- // https://github.com/delta-io/delta/issues/6167
- // XTable tracking issue: https://github.com/apache/incubator-xtable/issues/819
- //
- // Unlike DeltaConversionTarget (which uses Delta Standalone with commit tag support),
- // DeltaKernelConversionTarget cannot retrieve commit tags from Delta Kernel's API.
- // Rather than silently scanning all commits (O(n) performance cost) and always returning
- // empty, we explicitly throw an exception to indicate this feature is unsupported.
- //
- // When Delta Kernel adds commit tag support, this method can be reimplemented to:
- // 1. Scan commit history using tableImpl.getChanges(engine, 0, currentVersion, actionSet)
- // 2. Extract tags from CommitInfo.tags MapValue
- // 3. Parse XTABLE_METADATA from tags and match sourceIdentifier
- throw new NotSupportedException(
- "Source-to-target commit identifier mapping is not supported in DeltaKernelConversionTarget. "
- + "Delta Kernel 4.0.0 does not support commit tags in commitInfo. "
- + "See: https://github.com/delta-io/delta/issues/6167");
+ if (sourceIdentifier == null || sourceIdentifier.trim().isEmpty()) {
+ return Optional.empty();
+ }
+
+ String sourceIdentifierDomain = getSourceIdentifierDomainName(sourceIdentifier);
+ Table table = Table.forPath(engine, basePath);
+ Snapshot latestSnapshot;
+ try {
+ latestSnapshot = table.getLatestSnapshot(engine);
+ } catch (TableNotFoundException e) {
+ return Optional.empty();
+ }
+
+ return ((SnapshotImpl) latestSnapshot)
+ .getLatestTransactionVersion(engine, sourceIdentifierDomain)
Review Comment:
`commitTransaction` records the mapping with `txn.addDomainMetadata(...)`, but this reads `getLatestTransactionVersion`, which returns the `SetTransaction` version for an application id rather than a domain, so the lookup never matches and `TestDeltaKernelSync.testSourceTargetIdMapping:364` fails on the first lookup.
In Kernel 4.0.0 the write and read pairs are `txn.addDomainMetadata(domain, value)` with `SnapshotImpl.getDomainMetadata(domain)`, and `txnBuilder.withTransactionId(engine, appId, version)` with `SnapshotImpl.getLatestTransactionVersion(engine, appId)`. Neither can store the target version, since that only comes back from `TransactionCommitResult.getVersion()` once `commit(...)` has returned. `DeltaConversionTarget` and `IcebergConversionTarget` both scan history for the `XTABLE_METADATA` payload and return the version where it matched, so would following that shape be the safest route here, even though it keeps the scan the old comment wanted to avoid?
##########
xtable-core/src/main/java/org/apache/xtable/delta/DeltaSchemaExtractor.java:
##########
@@ -84,25 +84,19 @@ private static Integer nestedFieldId(Metadata nestedIds, String path) {
return (int) nestedIds.getLong(path);
}
- public InternalSchema toInternalSchema(StructType structType) {
- return toInternalSchema(structType, null, false, null, null, null, null);
+ private static Integer nestedFieldId(
+ Metadata nestedIds, String nestedIdPath, String parquetChildName, String internalChildName) {
+ Integer fieldId = nestedFieldId(nestedIds, childIdPath(nestedIdPath, parquetChildName));
+ if (fieldId != null || parquetChildName.equals(internalChildName)) {
+ return fieldId;
+ }
+ return nestedFieldId(nestedIds, childIdPath(nestedIdPath, internalChildName));
}
- private InternalSchema toInternalSchema(
- DataType dataType,
- String parentPath,
- boolean nullable,
- String comment,
- Metadata originalMetadata) {
- return toInternalSchema(dataType, parentPath, nullable, comment, originalMetadata, null, null);
+ public InternalSchema toInternalSchema(StructType structType) {
+ return toInternalSchema(structType, null, false, null, null, null, null);
}
- /**
- * @param nestedIds the {@code delta.columnMapping.nested.ids} metadata of the enclosing struct
- * field, or null when the field carries none
- * @param nestedIdPath the path of the current type within that metadata, starting at the
- * enclosing field's physical name
- */
private InternalSchema toInternalSchema(
Review Comment:
The `@param nestedIds` and `@param nestedIdPath` Javadoc added in #912 is dropped here, although the method still takes both. Was that intentional?
##########
xtable-core/src/main/java/org/apache/xtable/kernel/DeltaKernelConversionTarget.java:
##########
@@ -317,24 +314,26 @@ public String getTableFormat() {
@Override
public Optional<String> getTargetCommitIdentifier(String sourceIdentifier) {
- // Delta Kernel 4.0.0 does not support commit tags in commitInfo, which are required for
- // source-to-target commit identifier mapping. This limitation is documented in:
- // https://github.com/delta-io/delta/issues/6167
- // XTable tracking issue: https://github.com/apache/incubator-xtable/issues/819
- //
- // Unlike DeltaConversionTarget (which uses Delta Standalone with commit tag support),
- // DeltaKernelConversionTarget cannot retrieve commit tags from Delta Kernel's API.
- // Rather than silently scanning all commits (O(n) performance cost) and always returning
- // empty, we explicitly throw an exception to indicate this feature is unsupported.
- //
- // When Delta Kernel adds commit tag support, this method can be reimplemented to:
- // 1. Scan commit history using tableImpl.getChanges(engine, 0, currentVersion, actionSet)
- // 2. Extract tags from CommitInfo.tags MapValue
- // 3. Parse XTABLE_METADATA from tags and match sourceIdentifier
- throw new NotSupportedException(
- "Source-to-target commit identifier mapping is not supported in DeltaKernelConversionTarget. "
- + "Delta Kernel 4.0.0 does not support commit tags in commitInfo. "
- + "See: https://github.com/delta-io/delta/issues/6167");
+ if (sourceIdentifier == null || sourceIdentifier.trim().isEmpty()) {
+ return Optional.empty();
+ }
+
+ String sourceIdentifierDomain = getSourceIdentifierDomainName(sourceIdentifier);
+ Table table = Table.forPath(engine, basePath);
+ Snapshot latestSnapshot;
+ try {
+ latestSnapshot = table.getLatestSnapshot(engine);
+ } catch (TableNotFoundException e) {
+ return Optional.empty();
+ }
+
+ return ((SnapshotImpl) latestSnapshot)
+ .getLatestTransactionVersion(engine, sourceIdentifierDomain)
Review Comment:
@Akash3121 Can we confirm the behavior of delta-kernel through a test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org