Fix incorrect partitionValues_parsed with id & name column mapping in Delta Lake #24129
Conversation
| @@ -161,7 +161,7 @@ public void write(CheckpointEntries entries, TrinoOutputFile outputFile) | |||
| } | |||
| List<DeltaLakeColumnHandle> partitionColumns = extractPartitionColumns(entries.metadataEntry(), entries.protocolEntry(), typeManager); | |||
| List<RowType.Field> partitionValuesParsedFieldTypes = partitionColumns.stream() | |||
There was a problem hiding this comment.
Let's add a corresponding test in io.trino.plugin.deltalake.transactionlog.checkpoint.TestCheckpointWriter
There was a problem hiding this comment.
Can you share scenarios you want to cover in the class? I intentionally avoided that. Both TestCheckpointWriter & TestCheckpointEntryIterator are not suitable to verify partitionValues_parsed field because AddFileEntry doesn't hold the value.
There was a problem hiding this comment.
I was thinking about a test similar to io.trino.plugin.deltalake.transactionlog.checkpoint.TestCheckpointEntryIterator#testReadAddEntriesPartitionPruning with corresponding resource files
| @@ -183,7 +183,7 @@ public RowType getAddEntryType( | |||
| List<DeltaLakeColumnHandle> partitionColumns = extractPartitionColumns(metadataEntry, protocolEntry, typeManager); | |||
| if (!partitionColumns.isEmpty()) { | |||
| List<RowType.Field> partitionValuesParsed = partitionColumns.stream() | |||
| .map(column -> RowType.field(column.columnName(), typeManager.getType(getTypeSignature(DeltaHiveTypeTranslator.toHiveType(column.type()))))) | |||
There was a problem hiding this comment.
Add test in io.trino.plugin.deltalake.transactionlog.checkpoint.TestCheckpointEntryIterator
plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeBasic.java
Outdated
Show resolved
Hide resolved
ebyhr
force-pushed
the
ebi/delta-writer-partition-stats
branch
from
ee3627b
to
a60bbd3
Compare
| @@ -161,7 +161,7 @@ public void write(CheckpointEntries entries, TrinoOutputFile outputFile) | |||
| } | |||
| List<DeltaLakeColumnHandle> partitionColumns = extractPartitionColumns(entries.metadataEntry(), entries.protocolEntry(), typeManager); | |||
| List<RowType.Field> partitionValuesParsedFieldTypes = partitionColumns.stream() | |||
| .map(column -> RowType.field(column.columnName(), column.type())) | |||
| .map(column -> RowType.field(column.basePhysicalColumnName(), column.type())) | |||
There was a problem hiding this comment.
Looks good now
"PartitionValues": {
"col-6d32b73c-d46b-47f3-aeee-b4ce2231c81f": "30"
},
"PartitionValues_parsed": {
"Col456d32b73c45d46b4547f345aeee45b4ce2231c81f": 30
}
There was a problem hiding this comment.
Note the missing dashes.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| try (TestTable table = new TestTable( | ||
| getQueryRunner()::execute, | ||
| "test_checkpoint", | ||
| "(x int, part int) WITH (checkpoint_interval = 3, column_mapping_mode = '" + columnMappingMode + "', partitioned_by = ARRAY['part'])")) { |
There was a problem hiding this comment.
Can you add also test for other types (like Date) which has different representation in PartitionValues and PartitionValues_parsed
| testPartitionValuesParsedCheckpoint(ColumnMappingMode.NONE); | ||
| } | ||
|
|
||
| private void testPartitionValuesParsedCheckpoint(ColumnMappingMode columnMappingMode) |
There was a problem hiding this comment.
Should we have also product test in TestDeltaLakeColumnMappingMode to check reading/writing checkpoints by trino/delta
|
(Changed to draft for avoiding accidental merge) |
Description
We should write physical column names in
partitionValues_parsedfield on checkpoint files.Fixes #24121
Release notes