slachiewicz commented on code in PR #908:
URL: https://github.com/apache/incubator-xtable/pull/908#discussion_r4055088898
##########
xtable-core/src/test/java/org/apache/xtable/delta/TestDeltaHelper.java:
##########
@@ -169,8 +180,17 @@ public void createTable(SparkSession sparkSession, String
tableName, String base
}
if (enableColumnMapping) {
tableBuilder.property("delta.minReaderVersion", "2");
- tableBuilder.property("delta.minWriterVersion", "5");
tableBuilder.property("delta.columnMapping.mode", "name");
+ // Delta 3.x writes parquet field ids for the top level columns but
assigns none to a map
+ // key, a map value or a list element. Iceberg reads a file that carries
any id by id
+ // alone and ignores the name mapping, so those children become
unresolvable.
+ // IcebergCompatV2 is what makes Delta assign and record them.
+ if (enableIcebergCompatV2) {
+ tableBuilder.property("delta.minWriterVersion", "7");
Review Comment:
The ordering you describe is right, but the cause is in the Delta writer,
not in what XTable links. Delta 2.4 strips `parquet.field.id` for name-mode
tables (`DeltaColumnMapping.scala`, `case NameMapping` removes it), so those
files carry no ids and Iceberg resolves them through the name mapping we set.
Delta 3.0+ writes the id for every struct field ("Delta spec requires writer to
always write field_id"), and only IcebergCompatV2 assigns and writes ids for
list elements and map keys/values (`rewriteFieldIdsForIceberg`,
`DeltaParquetWriteSupport`). Iceberg's `ReadConf` uses file ids as soon as any
field has one and never falls back to the name mapping for the rest, so a map
or list column without IcebergCompatV2 is unresolvable.
XTable does not write the source table's parquet files, so a name-mode table
with map/list columns written by any Delta 3.x engine produces exactly this
unreadable Iceberg table on main today; the bump only makes our test harness
write files the way production writers have since Delta 3.0. Writer version 5
vs 7 is not the lever: 5 is what column mapping needs, 7 is what any table
feature (IcebergCompatV2 included) needs, and what decides readability is
whether the files carry nested ids. There is no reader-side fix in Iceberg
1.9.2 (name mapping is only applied when the file has no ids at all).
I'd keep the helper on IcebergCompatV2 for the readable path, keep
`testColumnMappingWithoutIcebergCompatIsNotReadableAsIceberg` pinning the other
one, and decide in #911 whether the source should warn or refuse such tables; a
hard fail would also hit tables written by Delta 2.x, which still convert fine.
##########
xtable-core/src/test/java/org/apache/xtable/delta/TestDeltaSync.java:
##########
@@ -358,7 +358,7 @@ public void
testTimestampPartitioning(PartitionTransformType transformType) thro
.expr();
org.apache.spark.sql.delta.DeltaLog deltaLog =
org.apache.spark.sql.delta.DeltaLog.forTable(sparkSession,
basePath.toString());
- org.apache.spark.sql.delta.Snapshot snapshot =
deltaLog.getSnapshotAtInit().snapshot();
+ org.apache.spark.sql.delta.Snapshot snapshot = deltaLog.snapshot();
Review Comment:
Same `DeltaLog` instance (the cache key is the qualified log path) and
`OptimisticTransaction.commit` runs `updateAfterCommit` on it, so `snapshot()`
already reflected the sync here; `getSnapshotAtInit` was a fresh load from disk
in 2.4 and has no 3.x counterpart. Switched to `deltaLog.update(false,
Option.empty(), Option.empty())` since that is the documented way to get the
latest snapshot and the cost is one log listing.
##########
xtable-core/src/test/java/org/apache/xtable/delta/ITDeltaConversionSource.java:
##########
@@ -984,10 +982,11 @@ public void
reuseMetadataDisabledReconstructsSnapshotPerCommit() {
ValidationTestHelper.validateTableChanges(allActiveFiles, allTableChanges);
// With reuse disabled the pre-optimization behaviour is restored in full:
one snapshot
- // reconstruction per commit, and no second listing of the log for
commit-file mtimes.
+ // reconstruction per commit. The absence of the extra mtime listing is no
longer assertable
+ // here, because DeltaLog.getChanges itself delegates to getChangeLogFiles
as of Delta 3.x,
+ // so the spy cannot tell that call apart from one this class makes.
verify(spiedDeltaLog, times(commitsBacklog.getCommitsToProcess().size()))
- .getSnapshotAt(anyLong(), any());
- verify(spiedDeltaLog, never()).getChangeLogFiles(anyLong(), anyBoolean());
+ .getSnapshotAt(anyLong(), any(), any());
Review Comment:
The count is stable: `getChanges(start, failOnDataLoss)` calls the two-arg
`getChangeLogFiles` exactly once (`DeltaLog.scala:298` in 3.3.3), so `times(1)`
pins the delegation and would still catch an extra listing from
`DeltaIncrementalChangesState`.
--
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]
