github-actions[bot] commented on code in PR #67479:
URL: https://github.com/apache/doris/pull/67479#discussion_r3922700643
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/iceberg/source/IcebergScanNodeTest.java:
##########
@@ -2938,6 +2938,63 @@ public void
testHistoricalPredicateUsesSelectedScanSchema() throws Exception {
Mockito.verify(scan).filter(Mockito.argThat(expression ->
expression.toString().contains("old_name")));
}
+ @Test
+ public void testHistoricalPredicatePlansAfterColumnRename() throws
Exception {
+ assertHistoricalPredicatePlansAfterSchemaEvolution(false);
+ }
+
+ @Test
+ public void testHistoricalPredicatePlansAfterColumnDrop() throws Exception
{
+ assertHistoricalPredicatePlansAfterSchemaEvolution(true);
+ }
+
+ private void assertHistoricalPredicatePlansAfterSchemaEvolution(boolean
dropColumn) throws Exception {
+ Schema historicalSchema = new Schema(
+ Types.NestedField.optional(1, "x", Types.IntegerType.get()),
+ Types.NestedField.optional(2, "y", Types.IntegerType.get()),
+ Types.NestedField.optional(3, "part",
Types.IntegerType.get()));
+ HadoopTables tables = new HadoopTables(new Configuration());
+ String tableLocation = temporaryFolder.getRoot().toPath()
+ .resolve("historical_predicate_after_" + (dropColumn ? "drop"
: "rename")).toUri().toString();
+ Table table = tables.create(
+ historicalSchema, PartitionSpec.unpartitioned(),
SortOrder.unsorted(),
+ ImmutableMap.of(TableProperties.FORMAT_VERSION, "2"),
tableLocation);
+ DataFile historicalDataFile = DataFiles.builder(table.spec())
+ .withPath(tableLocation + "/data/historical.parquet")
+ .withFormat(FileFormat.PARQUET)
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build();
+ table.newFastAppend().appendFile(historicalDataFile).commit();
+ long historicalSnapshotId = table.currentSnapshot().snapshotId();
+ int historicalSchemaId = table.currentSnapshot().schemaId();
+
+ if (dropColumn) {
+ table.updateSchema().deleteColumn("x").commit();
+ } else {
+ table.updateSchema().renameColumn("x", "renamed_x").commit();
+ }
+ DataFile currentDataFile = DataFiles.builder(table.spec())
Review Comment:
[P1] Cover schema-only evolution before advancing the snapshot
Iceberg schema commits do not create a new snapshot, so immediately after
this rename/drop `historicalSnapshotId` is still the table's current snapshot.
In Iceberg 1.11, `useSnapshot` selects that snapshot's old schema, but
`SnapshotScan.specs()` skips rebinding when the selected and current snapshot
IDs are equal and returns specs bound to the renamed/dropped current schema.
The old-name predicate then still fails in `Projections`. This append makes the
IDs differ, so both tests avoid the unresolved case while the release note
claims it is fixed. Please assert planning before this append and fix that path
as well.
##########
fe/fe-core/src/main/java/org/apache/iceberg/DeleteFileIndex.java:
##########
@@ -396,6 +400,11 @@ Builder afterSequenceNumber(long seq) {
return this;
}
+ Builder schemasById(Map<Integer, Schema> newSchemasById) {
Review Comment:
[P2] Expose and wire the historical-schema map
The only Doris caller of the public iterable builder is in another package,
so it cannot call this new package-private setter and currently supplies only
`icebergTable.specs()`. Iceberg 1.11 added `schemasById` specifically because
current specs no longer contain an equality-delete field after that field is
dropped; `fieldLookup` then returns null and `forDataFile` throws. Doris
catches that in the manifest-cache planner and reruns native planning, so every
affected scan loses the enabled cache and logs a failure. Please make this hook
public, pass the frozen table's full `schemas()` map, and cover a dropped
equality key through the cache planner.
##########
fe/pom.xml:
##########
@@ -226,7 +226,7 @@ under the License.
<module>fe-authentication</module>
</modules>
<properties>
-
<doris.hive.catalog.shade.version>3.1.2</doris.hive.catalog.shade.version>
+
<doris.hive.catalog.shade.version>3.1.3-ICEBERG-SNAPSHOT</doris.hive.catalog.shade.version>
Review Comment:
[P1] Pin an immutable shade release before merging
This changes packaged/runtime consumers to `3.1.3-ICEBERG-SNAPSHOT`, and
this POM configures snapshots with `updatePolicy=always`. The coordinate
currently resolves to timestamped build `3.1.3-ICEBERG-20260903.063704-1`, but
a later build can silently replace it and repository cleanup can make the same
Doris revision stop resolving; there is no stable 3.1.3 artifact yet, and the
PR description calls this temporary. Please publish the merged Doris Shade
change and pin its immutable release here before merging.
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/iceberg/source/IcebergScanNodeTest.java:
##########
@@ -2938,6 +2938,63 @@ public void
testHistoricalPredicateUsesSelectedScanSchema() throws Exception {
Mockito.verify(scan).filter(Mockito.argThat(expression ->
expression.toString().contains("old_name")));
}
+ @Test
+ public void testHistoricalPredicatePlansAfterColumnRename() throws
Exception {
+ assertHistoricalPredicatePlansAfterSchemaEvolution(false);
+ }
+
+ @Test
+ public void testHistoricalPredicatePlansAfterColumnDrop() throws Exception
{
+ assertHistoricalPredicatePlansAfterSchemaEvolution(true);
+ }
+
+ private void assertHistoricalPredicatePlansAfterSchemaEvolution(boolean
dropColumn) throws Exception {
+ Schema historicalSchema = new Schema(
+ Types.NestedField.optional(1, "x", Types.IntegerType.get()),
+ Types.NestedField.optional(2, "y", Types.IntegerType.get()),
+ Types.NestedField.optional(3, "part",
Types.IntegerType.get()));
+ HadoopTables tables = new HadoopTables(new Configuration());
+ String tableLocation = temporaryFolder.getRoot().toPath()
+ .resolve("historical_predicate_after_" + (dropColumn ? "drop"
: "rename")).toUri().toString();
+ Table table = tables.create(
+ historicalSchema, PartitionSpec.unpartitioned(),
SortOrder.unsorted(),
+ ImmutableMap.of(TableProperties.FORMAT_VERSION, "2"),
tableLocation);
+ DataFile historicalDataFile = DataFiles.builder(table.spec())
+ .withPath(tableLocation + "/data/historical.parquet")
+ .withFormat(FileFormat.PARQUET)
+ .withFileSizeInBytes(10)
+ .withRecordCount(2)
+ .build();
+ table.newFastAppend().appendFile(historicalDataFile).commit();
+ long historicalSnapshotId = table.currentSnapshot().snapshotId();
+ int historicalSchemaId = table.currentSnapshot().schemaId();
+
+ if (dropColumn) {
+ table.updateSchema().deleteColumn("x").commit();
+ } else {
+ table.updateSchema().renameColumn("x", "renamed_x").commit();
+ }
+ DataFile currentDataFile = DataFiles.builder(table.spec())
+ .withPath(tableLocation + "/data/current.parquet")
+ .withFormat(FileFormat.PARQUET)
+ .withFileSizeInBytes(10)
+ .withRecordCount(1)
+ .build();
+ table.newFastAppend().appendFile(currentDataFile).commit();
+
+ // Historical filters must be resolved with the snapshot schema after
later schema evolution.
+ TableScan scan = table.newScan()
+ .useSnapshot(historicalSnapshotId)
+ .project(table.schemas().get(historicalSchemaId));
+ BinaryPredicate conjunct = new
BinaryPredicate(BinaryPredicate.Operator.EQ,
+ new SlotRef(new TableName(), "x"), new IntLiteral(1,
Type.INT));
+ org.apache.iceberg.expressions.Expression predicate =
+ IcebergUtils.convertToIcebergExpr(conjunct, scan.schema());
+ Assert.assertNotNull(predicate);
+ scan = scan.filter(predicate);
+ Assert.assertEquals(1, materializeTasks(scan).size());
Review Comment:
[P1] Exercise the Doris planner, not only native planFiles
This assertion calls Iceberg's `TableScan.planFiles()` directly, but
production reaches `IcebergScanNode.isBatchMode()` first and batch mode
defaults on. That preflight calls `getMatchingManifest(...,
icebergTable.specs(), scan.filter())`; these specs use the current
renamed/dropped schema, so the historical old-name predicate fails while
`Projections` binds it, and the catch at `isBatchMode()` rethrows. Thus these
tests can pass while the default production query still fails. Rebind the
preflight/custom-planner specs to `scan.schema()` (matching Iceberg 1.11's
snapshot-spec behavior) and run this regression through the Doris node.
--
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]
