Gabriel39 commented on code in PR #67687:
URL: https://github.com/apache/doris/pull/67687#discussion_r3976181179
##########
regression-test/suites/external_table_p0/iceberg/iceberg_schema_change_ddl_with_branch.groovy:
##########
@@ -198,10 +198,11 @@ suite("iceberg_schema_change_ddl_with_branch",
"p0,external") {
// All branches expose the current table columns: id, name, grade, email,
phone.
- // Verify all branches have the latest columns
- qt_all_branches_have_grade """ SELECT id, grade FROM
${branch_table_name}@branch(branch1) WHERE grade > 0 ORDER BY id """
+ // Iceberg validates filters against the referenced snapshot schema, so
columns renamed or
+ // added later are verified through projection instead of predicates on
historical branches.
+ qt_all_branches_have_grade """ SELECT id, grade FROM
${branch_table_name}@branch(branch1) ORDER BY id """
Review Comment:
Fixed in d10663e032. The grade and phone predicates were restored in the
branch regression, and a focused unit test verifies that a predicate on a
renamed current-schema column plans against a historical branch head.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -2754,16 +2760,17 @@ private CloseableIterable<FileScanTask>
cacheBackedFileScanTasks(TableScan scan,
if (snapshot == null) {
return CloseableIterable.withNoopClose(Collections.emptyList());
}
- Expression filterExpr = combineFilter(filter, table, session);
- Map<Integer, PartitionSpec> specsById = table.specs();
+ Schema scanSchema = scan.schema();
Review Comment:
Fixed in 3f775f5c76. buildScan now reapplies the handle-selected logical
schema after snapshot selection. The regression uses a v3 table, drops and
re-adds a same-named required field with an initial default without advancing
the snapshot, enables the manifest cache, and verifies the matching old file is
retained without SDK fallback.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergPartitionUtils.java:
##########
@@ -805,11 +811,10 @@ private static IcebergRawPartition
generateRawPartition(Table table, StructLike
for (int i = 0; i < partitionSpec.fields().size(); ++i) {
PartitionField partitionField = partitionSpec.fields().get(i);
Class<?> fieldClass = partitionSpec.javaClasses()[i];
- int fieldId = partitionField.fieldId();
- // Iceberg partition field id starts at PARTITION_DATA_ID_START,
so the index into partitionData is
- // fieldId - PARTITION_DATA_ID_START.
- int index = fieldId - PARTITION_DATA_ID_START;
- Object o = partitionData.get(index, fieldClass);
+ // Iceberg 1.11 projects every metadata row into the table-wide
unified partition struct; a spec-local
+ // position can therefore point at a different evolved field, so
resolve the ordinal by field ID.
+ Integer ordinal =
partitionFieldOrdinals.get(partitionField.fieldId());
+ Object o = ordinal == null ? null : partitionData.get(ordinal,
fieldClass);
Review Comment:
Fixed in 3f775f5c76. Missing unified ordinals are now treated as
unrepresentable and return the established safe empty partition display. The
regression uses two distinct old buckets, verifies their exact names before the
source drop, and verifies empty output afterward. The cache key now includes
schemaId and specId as well as snapshotId, so the schema-only commit forces a
fresh metadata evaluation.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/iceberg/SchemaAwareDataTableScan.java:
##########
@@ -0,0 +1,57 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.iceberg;
+
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.Map;
+
+/** A data-table scan that keeps partition specs bound to the scan's schema. */
+public final class SchemaAwareDataTableScan extends DataTableScan {
+
+ private SchemaAwareDataTableScan(Table table, Schema schema,
TableScanContext context) {
+ super(table, schema, context);
+ }
+
+ public static TableScan newScan(Table table) {
+ return new SchemaAwareDataTableScan(table, table.schema(),
TableScanContext.empty());
+ }
+
+ /** Returns every table spec rebound to {@code schema}. */
+ public static Map<Integer, PartitionSpec> specsFor(Table table, Schema
schema) {
+ if (schema.sameSchema(table.schema())) {
+ return table.specs();
+ }
+
+ Map<Integer, PartitionSpec> specs = new LinkedHashMap<>();
+ table.specs().forEach((id, spec) -> specs.put(id,
spec.toUnbound().bind(schema, true)));
+ return Collections.unmodifiableMap(specs);
+ }
+
+ @Override
+ protected Map<Integer, PartitionSpec> specs() {
+ // A metadata-only schema commit preserves the current snapshot ID, so
schema identity—not snapshot
+ // identity—must decide whether historical partition specs need
rebinding.
+ return specsFor(table(), tableSchema());
Review Comment:
Fixed in 3f775f5c76. streamingSplitEstimate now uses
SchemaAwareDataTableScan.specsFor(table, scan.schema()). The test executes the
estimator followed by streaming dispatch both in the equal-snapshot schema-only
state and after a later append advances the current snapshot.
--
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]
