You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@xtable.apache.org by "Mola-maker (via GitHub)" <gi...@apache.org> on 2026/10/20 18:04:30 UTC
[PR] fix(core): resolve Delta partition values by storage name when column mapping is enabled [incubator-xtable]
Mola-maker opened a new pull request, #973:
URL: https://github.com/apache/incubator-xtable/pull/973
Fixes #972
## What is the purpose of the pull request
With `delta.columnMapping.mode=name`, Delta writes the `partitionValues` of add/remove actions keyed by each column's physical name (`col-<uuid>`), while XTable's internal partition fields carry the logical name. `DeltaKernelPartitionExtractor` and `DeltaPartitionExtractor` looked values up by the logical name only, so every identity partition value came out `null` — the sync reported SUCCESS while the converted Iceberg table had NULL partition columns (e.g. `WHERE country='USA'` matched 0 rows instead of 2).
Two gaps combined into the bug on the Kernel path: `DeltaKernelSchemaExtractor` computed each field's physical (storage) name from `delta.columnMapping.physicalName` but never stored it on the built `InternalField` (the Spark-based `DeltaSchemaExtractor` does set it), and neither partition extractor consulted `InternalField.getStorageName()` when resolving partition values.
## Brief change log
- `DeltaKernelSchemaExtractor`: set the already-computed storage name on the built `InternalField`, mirroring `DeltaSchemaExtractor`.
- `DeltaKernelPartitionExtractor` / `DeltaPartitionExtractor`: in `getSerializedPartitionValue`, resolve identity partition values by the field's storage name first when one is present, falling back to the logical name.
- Tests: added `testPartitionValueExtractionWithColumnMappingPhysicalName` to `TestDeltaKernelPartitionExtractor` and `TestDeltaPartitionExtractor`, feeding a partition-values map keyed by the physical name.
## Verify this pull request
This change added tests and can be verified as follows:
- Before the fix, the new extraction test fails: expected partition range `USA`, actual `null`. Verified in CI (fork run of this branch without the fix): xtable-core ran 550 tests with this single failure.
- After the fix, the same CI build (`Maven CI Build` on the fork PR) passes: xtable-core `Tests run: 550, Failures: 0, Errors: 0, Skipped: 3`, BUILD SUCCESS. Red/green runs: https://github.com/Mola-maker/incubator-xtable/pull/1
- `./mvnw -pl xtable-core -Dtest=TestDeltaKernelPartitionExtractor,TestDeltaPartitionExtractor 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
Re: [PR] fix(core): resolve Delta partition values by storage name when column mapping is enabled [incubator-xtable]
Posted by "Mola-maker (via GitHub)" <gi...@apache.org>.
Mola-maker commented on PR #973:
URL: https://github.com/apache/incubator-xtable/pull/973#issuecomment-6409795922
Hi @the-other-tim-brown, thanks for the review. I've updated the integration test testColumnMappingEnabledDeltaToIceberg as requested so the column-mapped Delta table is now partitioned by level (commit c5fae2fe, on fork branch fix/972-column-mapping-partition-values).
Fork CI for that commit is green:
- Maven CI Build: success (https://github.com/Mola-maker/incubator-xtable/actions/runs/40272831132)
- Spark Runtime Bundle Validation: success (https://github.com/Mola-maker/incubator-xtable/actions/runs/40272831097)
Please let me know if you'd like anything else adjusted.
--
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(core): resolve Delta partition values by storage name when column mapping is enabled [incubator-xtable]
Posted by "Mola-maker (via GitHub)" <gi...@apache.org>.
Mola-maker commented on PR #973:
URL: https://github.com/apache/incubator-xtable/pull/973#issuecomment-6409982327
Added a regression test for #977 in 11720083: `testColumnMappingPhysicalNamePropagatedAsStorageName` in `TestDeltaKernelSchemaExtractor` verifies that a column renamed after files were written keeps its pre-rename physical name as `storageName`, so the Iceberg name mapping can resolve files written before the rename. The test fails on main (storageName is null) and passes with the `storageName` propagation already in this PR. No production code changed in this push — test only.
--
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(core): resolve Delta partition values by storage name when column mapping is enabled [incubator-xtable]
Posted by "vaibhavk1992 (via GitHub)" <gi...@apache.org>.
vaibhavk1992 commented on PR #973:
URL: https://github.com/apache/incubator-xtable/pull/973#issuecomment-6410481886
Thanks @Mola-maker for this! I verified the storageName change end to end for #977 on main + this PR's production diff. Delta table with delta.columnMapping.mode=name enabled after the first insert, then RENAME COLUMN name TO new_name, synced to Iceberg with DeltaKernelConversionSourceProvider
A few things I found while testing:
1. Null partition value can pick up another column's value
(DeltaKernelPartitionExtractor.java L320, same code in DeltaPartitionExtractor.java L327)
If a partition value is null, Delta writes the physical key with a null value. values.get(storageName) then returns null, and the code falls back to the logical name. When column mapping is enabled late, the physical names are the original column names, so that logical name can match another column's physical key and pick up its value.
Repro (same result on both sources):
CREATE TABLE t (id INT, a STRING, b STRING) USING DELTA PARTITIONED BY (a, b);
ALTER TABLE t SET TBLPROPERTIES ('delta.minReaderVersion'='2','delta.minWriterVersion'='5','delta.columnMapping.mode'='name');
ALTER TABLE t RENAME COLUMN a TO tmp; -- tmp -> physical "a"
ALTER TABLE t RENAME COLUMN b TO a; -- a -> physical "b"
INSERT INTO t (id, tmp, a) VALUES (1, 'X', NULL);
Delta returns (1, X, NULL). With this PR, partition value extraction returns tmp=X, a=X. The value for a should be null.
Suggestion: once a storage name exists, treat it as authoritative:
if (storageName != null && values.containsKey(storageName)) {
return values.get(storageName);
}
return values.getOrDefault(partitionField.getSourceField().getName(), null);
With that change the repro returns tmp=X, a=null, and the existing TestDeltaKernelPartitionExtractor, TestDeltaPartitionExtractor and TestDeltaKernelSchemaExtractor tests still pass.
2. Test coverage for the Kernel path
ITConversionController.testColumnMappingEnabledDeltaToIceberg uses getConversionSourceProvider(DELTA), which always returns the Standalone DeltaConversionSourceProvider. It also enables column mapping at table creation. So no integration test runs the #977 scenario (mapping enabled after data exists, then rename) through the Kernel source, and the TestDeltaKernelSchemaExtractor test only asserts that storageName is set. Could you add an IT that covers the Kernel source with late-enabled mapping and a rename, checking the data read back from Iceberg? Running it against both sources would also cover #886.
3. Separate follow-up, not blocking: generated partition columns
With column mapping enabled, a generated partition column (yr INT GENERATED ALWAYS AS (YEAR(ts)), partitioned by yr) still comes out null on both the Kernel and Standalone sources, with and without this PR. The same table without column mapping resolves correctly. The partitionFieldNames branch of getSerializedPartitionValue looks values up by the generated column's logical name, and the generated column isn't in the internal schema, so there's no storageName to use. This is outside the scope of #972, so I'm happy for it to be a separate issue.
--
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(core): resolve Delta partition values by storage name when column mapping is enabled [incubator-xtable]
Posted by "Mola-maker (via GitHub)" <gi...@apache.org>.
Mola-maker commented on PR #973:
URL: https://github.com/apache/incubator-xtable/pull/973#issuecomment-6408737957
Addressed in c5fae2f: `testColumnMappingEnabledDeltaToIceberg` now creates the column-mapping Delta table partitioned by `level` and passes the `level:VALUE` partition config to the sync config, so the integration test covers the column-mapping partition-value path (partition values are compared row by row in `checkDatasetEquivalence`). CI is running on the updated branch.
--
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(core): resolve Delta partition values by storage name when column mapping is enabled [incubator-xtable]
Posted by "Mola-maker (via GitHub)" <gi...@apache.org>.
Mola-maker commented on PR #973:
URL: https://github.com/apache/incubator-xtable/pull/973#issuecomment-6412382518
Thanks for verifying end to end and for the precise repro — all three points addressed in the latest push (commit cf7a2043):
1. Null partition value: fixed exactly as suggested, in both DeltaPartitionExtractor and DeltaKernelPartitionExtractor — once the map contains the storage key, its value (including null) is authoritative and no longer falls back to the logical name. Regression tests added to TestDeltaPartitionExtractor and TestDeltaKernelPartitionExtractor mirroring the rename collision (a null under the storage key must not resolve to the logical name's entry).
2. Kernel-path IT: added testLateColumnMappingRenameDeltaToIceberg, run against both the Kernel and the Standalone Delta sources. It enables column mapping after data exists, applies the double rename (level -> tmp, street -> level), inserts a row with tmp=X and a null level partition value, syncs to Iceberg, and asserts the Iceberg row reads back tmp=X, level=NULL (plus full dataset equivalence). Running both sources also covers #886.
3. Generated partition columns: left as a separate follow-up as you suggested, not in this PR.
One environment note: I can't run the JVM tests locally in this environment, so the new unit tests and ITs rely on CI here — the fork CI run for this push will tell us.
--
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