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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
