github-actions[bot] commented on code in PR #67687:
URL: https://github.com/apache/doris/pull/67687#discussion_r3976057238
##########
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:
[P1] Preserve the handle-selected schema in the cache planner. A normal
`beginQuerySnapshot` pin can carry the current schema ID with the preceding
snapshot ID after a metadata-only schema commit; the native `useSnapshot` then
resets `scan.schema()` to that snapshot's old schema. On a v3 table, drop a
non-null `k` and add a new same-named `k` with an initial default without
creating a snapshot: `WHERE k = DEFAULT_VALUE` must retain old files so BE can
materialize the new default, but these evaluators bind `k` to the retired field
ID and can silently prune the files from old metrics. No exception reaches SDK
fallback. Please carry the handle-selected logical schema into cache planning
and cover this production latest-pin case with the manifest cache enabled.
##########
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:
[P1] Apply this rebound spec map to the streaming batch estimator as well.
`doInitialize` pins the historical handle before `computeBatchMode`, so
production scans call `streamingSplitEstimate`; that path builds this scan but
manually passes `table.specs()` to `getMatchingManifest` instead of using the
scan-bound specs. In the metadata-only rename/drop state fixed here, projecting
the historical-name filter through those current-schema specs throws during the
batch estimate, before either `planScan` or `streamSplits` can use this
override. The new tests call `planScan` directly and miss this dispatch path.
Please reuse `specsFor(table, scan.schema())` there and cover
estimator/full-dispatch behavior both before and after the snapshot ID advances.
##########
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:
[P2] Treat a missing unified ordinal as unrepresentable, not as a real null
partition. Iceberg 1.11 excludes a historical partition field once its source
column is dropped; its `$partitions` table coerces files into that reduced
struct and groups by it, so multiple distinct old buckets collapse into one
metadata row. This line then fabricates `region_bucket=null`, corrupting
partition names/values. The new test has one old bucket and checks only total
size, so it misses both failures. Please preserve spec-local historical values
or keep the prior safe unpartitioned/empty fallback, cover two old buckets with
exact name/value assertions, and account for the raw cache being keyed only by
snapshot ID when schema changes without a 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]
