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