github-actions[bot] commented on code in PR #68259:
URL: https://github.com/apache/doris/pull/68259#discussion_r4139584179
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/iceberg/source/IcebergScanNodeTest.java:
##########
@@ -1816,6 +1816,68 @@ public void
testSchemaCarrierKeepsDroppedEqualityFieldDefault() throws Exception
IcebergUtils.getSerializedInitialDefaults(fields,
false).get(7));
}
+ @Test
+ public void
testSplitKeepsOldSpecIdentityValuesAfterEvolvingToUnpartitioned() throws
Exception {
+ // A file written under identity(p) keeps p=7 only in its manifest
partition metadata. Gating on
+ // the CURRENT spec drops that value once the default spec evolves to
unpartitioned, and keying
+ // the partition-key classification off the columns common to all
specs leaves BE reading p from
+ // a file that does not store it, so the column comes back NULL in
both cases.
+ Schema schema = new Schema(
+ Types.NestedField.required(1, "id", Types.IntegerType.get()),
+ Types.NestedField.required(2, "p", Types.IntegerType.get()));
+ PartitionSpec spec =
PartitionSpec.builderFor(schema).identity("p").build();
+ String location =
temporaryFolder.newFolder("identity_evolved_table").toURI().toString();
+ Table table = new HadoopTables(new Configuration()).create(schema,
spec, location);
+ table.newFastAppend()
+ .appendFile(DataFiles.builder(table.spec())
+ .withPath(location + "data/p=7/old.parquet")
+ .withFileSizeInBytes(1024L)
+ .withRecordCount(2L)
+ .withFormat(FileFormat.PARQUET)
+ .withPartitionPath("p=7")
+ .build())
+ .commit();
+ table.updateSpec().removeField("p").commit();
+ Assert.assertTrue(table.spec().isUnpartitioned());
+
+ SessionVariable sessionVariable = new SessionVariable();
+ sessionVariable.enableFileScannerV2 = true;
+ TestIcebergScanNode node = new TestIcebergScanNode(sessionVariable);
+ setIcebergTable(node, table);
+ setPrivateField(node, "formatVersion", 2);
+ setPrivateField(node, "storagePropertiesMap", Collections.emptyMap());
+ setPrivateField(node, "partitionMapInfos", new HashMap<>());
+ // Exactly what doInitialize computes for this table.
+ setPrivateField(node, "isPartitionedTable",
table.spec().isPartitioned());
+ setPrivateField(node, "hasPartitionedSpec",
IcebergUtils.hasPartitionedSpec(table));
+
+ // p must stay a partition key even though the current spec no longer
mentions it; otherwise BE
+ // ignores the split value and reads the (absent) physical column.
+ Assert.assertEquals(ImmutableList.of("p"),
orderedPathPartitionKeys(node));
+
+ FileScanTask task;
+ try (CloseableIterable<FileScanTask> tasks =
table.newScan().planFiles()) {
+ Iterator<FileScanTask> iterator = tasks.iterator();
+ Assert.assertTrue(iterator.hasNext());
+ task = iterator.next();
+ }
+
+ IcebergSplit split = createIcebergSplit(node, task);
+ Assert.assertEquals(ImmutableMap.of("p", "7"),
split.getIcebergPartitionValues());
+ Assert.assertEquals(Integer.valueOf(0), split.getPartitionSpecId());
+ Assert.assertEquals("[\"7\"]", split.getPartitionDataJson());
Review Comment:
[P1] Do not expect row-ID partition JSON in this read split. This test
creates an empty scan tuple, so `requiresRowId()` is false and
`createIcebergSplit()` leaves `partitionDataJson` null; the assertion always
fails before it checks the Thrift range. Assert its absence here (as
`testReadSplitsSkipPartitionJson` does), and keep JSON assertions in a test
that projects `_row_id`.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/source/IcebergScanNode.java:
##########
@@ -2626,7 +2645,7 @@ private Split createIcebergSplit(FileScanTask
fileScanTask) throws UserException
+ specId + ". Rewrite historical data files before
DELETE or UPDATE.", e);
}
}
- if (isPartitionedTable) {
+ if (hasPartitionedSpec) {
Review Comment:
[P2] Avoid caching empty maps for old transformed-only partitions. If the
current spec is unpartitioned but a historical spec has only bucket/truncate
fields, `hasPartitionedSpec` enters this branch for every old file;
`getIdentityPartitionInfoMap()` returns empty, so no split value is sent, and
`scannedPartitionNum()` ignores the cache. The map still retains one `(specId,
partitionData)` key per distinct old partition, making FE memory grow with
partition cardinality even during streaming batch planning. Skip caching those
empty maps when they are not needed for partition counts.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
