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