You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@xtable.apache.org by "slachiewicz (via GitHub)" <gi...@apache.org> on 2026/08/23 10:56:57 UTC
[PR] [902] Move the build to Spark 3.5 and Delta 3.3 [incubator-xtable]
slachiewicz opened a new pull request, #908:
URL: https://github.com/apache/incubator-xtable/pull/908
Part of #902, which folds the skipped Spark 3.5 step into the Spark 4.0 work. This is that step on its own: Spark 3.5 and Delta 3.3 with Scala still on 2.12 and the compiler target still Java 8, so the Delta 2.4 to 3.x jump can be reviewed apart from the Scala 2.13 and Java 17 flip that follows.
## What is the purpose of the pull request
Moves the build to `spark.version` 3.5.9 and `delta.version` 3.3.3, and ports the Delta call sites that 3.x moved. Hudi 1.2.0, Iceberg 1.9.2 and Paimon 1.3.1 all publish their 3.5 variants, so no engine version changes. `delta-spark_2.12:3.3.3` is still class-file 52, so Java 8 remains a valid target and CI stays on JDK 11.
## Brief change log
- Renamed `io.delta:delta-core_` to `io.delta:delta-spark_`, which Delta did in 3.0, across the four consuming modules, the `xtable-utilities` shade include, the demo dependency scripts and two docs snippets that still named Delta 2.x.
- `AddFile` gained `baseRowId`, `defaultRowCommitVersion` and `clusteringProvider`, so its constructor takes 11 arguments; the sync leaves all three unset.
- `DeltaLog.getSnapshotAt` now takes a catalog table, and `ensureLogDirectoryExist` is `createLogDirectoriesIfNotExists`.
- `reuseMetadataDisabledReconstructsSnapshotPerCommit` no longer asserts that `getChangeLogFiles` goes uncalled: as of Delta 3.x `DeltaLog.getChanges` delegates to it, so the spy cannot tell that call apart from one `DeltaIncrementalChangesState` makes. The snapshot-per-commit assertion it shares the test with is unchanged.
- `ITDeltaDeleteVectorConvert` reloads its table handle after enabling deletion vectors. Delta 3.x exposes the `row_index` metadata column only when deletion vectors are readable on the snapshot the scan was planned against (`DeltaParquetFileFormat.scala:255-262`), and the handle resolved at construction still carries the pre-`ALTER TABLE` protocol.
## Verify this pull request
Built and run locally on JDK 11: 494 unit tests pass.
Two integration failures are outstanding, which is why this is a draft:
- `ITDeltaDeleteVectorConvert` reaches `validateDeletedRecordCount` and finds one file carrying a deletion vector where the test expects none, because Delta 3.x persists deletion vectors for `MERGE`. The expectations in that test are cumulative, so they need re-deriving against the new behaviour, or the merge DV persistence needs turning off for that phase. Worth a maintainer opinion on which.
- `ITConversionController#testColumnMappingEnabledDeltaToIceberg` fails in Iceberg's Parquet reader: the map key path it asks for carries a different column-mapping physical name than the data file holds. That points at the Delta to Iceberg schema conversion picking up a physical name from a different schema version, not at a version pin. Not yet diagnosed.
Nine further errors seen locally, all `NoSuchElementException: No value present in Option` from `scheduleCompaction` on MERGE_ON_READ tables in `ITHudiConversionSource` and `ITConversionController`, reproduce on unmodified `main` and are green in CI, so they are an artefact of this machine rather than of this change.
*This change was created with AI assistance.*
--
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] [902] Move the build to Spark 3.5 and Delta 3.3 [incubator-xtable]
Posted by "slachiewicz (via GitHub)" <gi...@apache.org>.
slachiewicz closed pull request #908: [902] Move the build to Spark 3.5 and Delta 3.3
URL: https://github.com/apache/incubator-xtable/pull/908
--
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] [902] Move the build to Spark 3.5.9 and Delta 3.3.3 [incubator-xtable]
Posted by "slachiewicz (via GitHub)" <gi...@apache.org>.
slachiewicz closed pull request #908: [902] Move the build to Spark 3.5.9 and Delta 3.3.3
URL: https://github.com/apache/incubator-xtable/pull/908
--
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] [902] Move the build to Spark 3.5.9 and Delta 3.3.3 [incubator-xtable]
Posted by "slachiewicz (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [902] Move the build to Spark 3.5.9 and Delta 3.3.3 [incubator-xtable]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #908:
URL: https://github.com/apache/incubator-xtable/pull/908#discussion_r4067697568
##########
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:
In the case where `parquet.field.id` is stripped, there is a default mapping that Iceberg can use to read the table still if I am remembering correctly. It applies that mapping to files written without field IDs. Not all engines support this Iceberg feature though.
I think if we are going to produce something that may not be able to be read, we should log out an error. If it definitely will not be able to be read, we should throw an exception.
--
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] [902] Move the build to Spark 3.5.9 and Delta 3.3.3 [incubator-xtable]
Posted by "vinishjail97 (via GitHub)" <gi...@apache.org>.
vinishjail97 commented on code in PR #908:
URL: https://github.com/apache/incubator-xtable/pull/908#discussion_r4052505873
##########
website/docs/how-to.md:
##########
@@ -55,7 +55,7 @@ values={[
```shell md title="shell"
pyspark \
- --packages org.apache.hudi:hudi-spark3.4-bundle_2.12:1.2.0 \
+ --packages org.apache.hudi:hudi-spark3.2-bundle_2.12:0.14.0 \
Review Comment:
This line goes to `hudi-spark3.2-bundle_2.12:0.14.0`, where `main` currently has `hudi-spark3.4-bundle_2.12:1.2.0`, so I think a rebase picked up an older revision of the file. With `spark.version.prefix` now `3.5`:
```suggestion
--packages org.apache.hudi:hudi-spark3.5-bundle_2.12:1.2.0 \
```
##########
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:
Is `snapshot()` guaranteed to be current at this point? It returns the unsafe volatile snapshot, so it reflects whatever was last cached rather than the state after `syncSnapshot`. `deltaLog.update(...)` looks like the closer replacement for `getSnapshotAtInit().snapshot()`, if the Java call site can supply the defaults.
##########
website/docs/hms.md:
##########
@@ -182,7 +182,7 @@ SELECT * FROM delta_db.<table_name>;
<TabItem value="iceberg">
```shell md title="shell"
-spark-sql --packages org.apache.iceberg:iceberg-spark-runtime-3.4_2.12:1.9.2 \
+spark-sql --packages org.apache.iceberg:iceberg-spark-runtime-3.2_2.12:1.2.1 \
Review Comment:
And here, where `main` has `iceberg-spark-runtime-3.4_2.12:1.9.2`.
```suggestion
spark-sql --packages org.apache.iceberg:iceberg-spark-runtime-3.5_2.12:1.9.2 \
```
##########
website/docs/how-to.md:
##########
@@ -74,7 +74,7 @@ pyspark \
```shell md title="shell"
pyspark \
- --packages org.apache.iceberg:iceberg-spark-runtime-3.4_2.12:1.9.2 \
+ --packages org.apache.iceberg:iceberg-spark-runtime-3.2_2.12:1.4.1 \
Review Comment:
Same revert here: `main` has `iceberg-spark-runtime-3.4_2.12:1.9.2`.
```suggestion
--packages org.apache.iceberg:iceberg-spark-runtime-3.5_2.12:1.9.2 \
```
##########
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:
This is the one point I would call blocking.
On `main`, `testColumnMappingEnabledDeltaToIceberg` passes against a writer-version-5 table with no IcebergCompatV2; after the bump that same table only converts readably once the helper sets it, which is what `testColumnMappingWithoutIcebergCompatIsNotReadableAsIceberg` now pins. If I have that ordering right, existing column mapped tables would start producing an unreadable Iceberg table while the sync still reports `SUCCESS`.
@slachiewicz Can we explore if there's any possibility to make this upgrade without introducing any regression? We can make this decision by doing some research on what's the most used and stable minWriterVersions in delta.
##########
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:
Rather than dropping the assertion, would a `times(...)` on `getChangeLogFiles` still pin the intent? The count contributed by the `getChanges` delegation should be fixed for a given backlog, so an extra listing from this class would still show up. Suggestion only, and happy to defer if the count turns out not to be stable.
##########
xtable-core/src/test/java/org/apache/xtable/ITConversionController.java:
##########
@@ -753,6 +757,42 @@ public void testIcebergCorruptedSnapshotRecovery() throws Exception {
}
}
+ // Pins what happens for a column mapped Delta table that does not enable IcebergCompatV2 and
+ // holds a map column. Delta 3.x writes parquet field ids for the top level columns only, so
+ // Iceberg resolves by id and never applies the name mapping, and a map key, which carries no
+ // id of its own, cannot be resolved. The sync itself reports success; the Iceberg table it
+ // leaves behind is unreadable. Whether XTable should instead refuse the sync is open in
+ // https://github.com/apache/incubator-xtable/issues/911.
+ @Test
+ public void testColumnMappingWithoutIcebergCompatIsNotReadableAsIceberg() {
+ String tableName = getTableName();
+ ConversionSourceProvider<?> conversionSourceProvider = getConversionSourceProvider(DELTA);
+ try (TestSparkDeltaTable table =
+ TestSparkDeltaTable.forColumnMappingWithoutIcebergCompat(
+ tableName, tempDir, sparkSession, null)) {
+ table.insertRows(20);
+ ConversionController conversionController =
+ new ConversionController(jsc.hadoopConfiguration());
+ ConversionConfig conversionConfig =
+ getTableSyncConfig(
+ DELTA,
+ SyncMode.INCREMENTAL,
+ tableName,
+ table,
+ Collections.singletonList(ICEBERG),
+ null,
+ null);
+ Map<String, SyncResult> results =
+ conversionController.sync(conversionConfig, conversionSourceProvider);
+ assertEquals(
+ SyncStatusCode.SUCCESS, results.get(ICEBERG).getTableFormatSyncStatus().getStatusCode());
+
+ assertThrows(
+ SparkException.class,
Review Comment:
`SparkException` is broad enough that this would also pass if the read failed for an unrelated reason. Would asserting on the cause or the message keep it pinned to the field-id case in #911?
##########
xtable-core/src/test/java/org/apache/xtable/delta/ITDeltaDeleteVectorConvert.java:
##########
@@ -68,6 +68,10 @@ public static void setupOnce() {
"spark.sql.catalog.spark_catalog",
"org.apache.spark.sql.delta.catalog.DeltaCatalog")
.config("spark.databricks.delta.retentionDurationCheck.enabled", "false")
+ // Delta 3.x persists deletion vectors for MERGE as well as DELETE. This test uses
+ // merges to model plain upserts and counts vectors only from the delete steps that
+ // follow, so keep merges on the rewrite path.
+ .config("spark.databricks.delta.merge.deletionVectors.persistent", "false")
Review Comment:
This keeps the test on the 2.4 write path, so the 3.x default of persisting a deletion vector for `MERGE` stays uncovered end to end. Would it be reasonable to add a second run at the default? Another option is parameterized tests for both versions?
--
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] [902] Move the build to Spark 3.5.9 and Delta 3.3.3 [incubator-xtable]
Posted by "slachiewicz (via GitHub)" <gi...@apache.org>.
slachiewicz commented on PR #908:
URL: https://github.com/apache/incubator-xtable/pull/908#issuecomment-5966769600
Closing in favour of the Spark 4 upgrade in #902. The open question in the `TestDeltaHelper` thread, column-mapped tables whose map or list columns carry no field ids, is tracked in #911. #778 bumps the same versions, and the Delta 3.x API changes on this branch remain available as a reference.
--
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