morrySnow commented on code in PR #66530:
URL: https://github.com/apache/doris/pull/66530#discussion_r3733119298
##########
fe/fe-connector/fe-connector-hudi/src/main/java/org/apache/doris/connector/hudi/HudiScanPlanProvider.java:
##########
@@ -1009,4 +1025,97 @@ static Map<String, String>
storageHadoopConfig(ConnectorContext context) {
private ConnectorStorageContext storage() {
return context.getStorageContext();
}
+
+ /** Package-private for offline unit tests of key construction. */
+ static HudiScanReuseKey hudiScanReuseKey(long catalogId, String queryId,
HudiTableHandle handle,
+ ConnectorScanRequest request) {
+ return new HudiScanReuseKey(catalogId, queryId, handle, request);
+ }
+
+ /**
+ * Statement-scoped cache key for one Hudi scan.
+ *
+ * <p>Includes every input that changes the planned split list: table
identity, the snapshot
+ * instant, the incremental window (begin/end instant + incremental
options), the pruned
+ * partition set, the partition keys, and the JNI metadata carriers (input
format / serde).
+ * Session variables are statement-constant and deliberately absent.
+ */
+ static final class HudiScanReuseKey {
+ private final long catalogId;
+ private final String queryId;
+ private final String dbName;
+ private final String tableName;
+ private final String basePath;
+ private final String queryInstant;
+ private final String beginInstant;
+ private final String endInstant;
+ private final Map<String, String> incrementalParams;
+ private final List<String> prunedPartitionPaths;
+ private final List<String> partitionKeyNames;
+ private final String inputFormat;
+ private final String serdeLib;
+
+ private HudiScanReuseKey(long catalogId, String queryId,
HudiTableHandle handle,
+ ConnectorScanRequest request) {
+ // The catalog id and query id isolate same-named tables across a
cross-catalog
Review Comment:
**Issue: `ConnectorScanRequest request` parameter accepted but never used.**
Same pattern as Hive — the constructor accepts `request` but only reads
fields from `handle`. Every field (catalogId, queryId, dbName, tableName,
basePath, queryInstant, beginInstant, endInstant, incrementalParams,
prunedPartitionPaths, partitionKeyNames, inputFormat, serdeLib) comes from
`handle`.
Consider either removing the unused parameter or documenting why
request-level fields (filter, columns, countPushdown) don't affect Hudi split
planning and are safely omitted from the key.
##########
fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorScanKeyUtils.java:
##########
@@ -0,0 +1,62 @@
+// 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.doris.connector.spi;
+
+import org.apache.doris.connector.spi.pushdown.ConnectorAnd;
+import org.apache.doris.connector.spi.pushdown.ConnectorExpression;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+
+/**
+ * Helpers for statement-scoped scan-reuse key construction.
+ */
+public final class ConnectorScanKeyUtils {
+
+ private ConnectorScanKeyUtils() {
+ }
+
+ /**
+ * Flatten AND conjuncts into an immutable list of {@link
ConnectorExpression}, so two
+ * semantically equal filters whose conjunct order differs still map to
the same reuse key.
+ * Each conjunct is compared by structural {@code equals} rather than by
{@code toString},
Review Comment:
**Issue: Javadoc claims order-independent matching, but implementation
doesn't sort.**
The javadoc states "two semantically equal filters whose conjunct order
differs still map to the same reuse key." However, `flattenConjuncts` only
flattens AND nodes — it does not sort. Since `List.equals` is order-sensitive:
- `flatten(A AND B)` → `[A, B]`
- `flatten(B AND A)` → `[B, A]`
These two lists are NOT equal, so they would NOT map to the same reuse key.
Fix: either (1) sort the flattened conjuncts to a canonical order (e.g., via
a structural comparator on `ConnectorExpression`), or (2) update the javadoc to
say the method only flattens, not canonicalizes across orderings. Given the
PR's goal of maximizing reuse, option (1) is preferred — but note that
`ConnectorExpression` would need a stable total order for sorting.
##########
fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveScanPlanProvider.java:
##########
@@ -719,4 +741,81 @@ private static final class PartitionScanInfo {
private ConnectorStorageContext storage() {
return context.getStorageContext();
}
+
+ /**
+ * Statement-scoped cache key for one Hive scan.
+ *
+ * <p>Includes every input that changes the planned split list: table
identity, the file formats
+ * (input format / serialization lib / JSON single-column gate), the
partition keys and the
+ * pruned partition set (each partition's location and values). ACID
tables are excluded
+ * upstream, and session variables are statement-constant, so both stay
out of the key.
+ */
+ private static final class HiveScanReuseKey {
+ private final long catalogId;
+ private final String queryId;
+ private final String dbName;
+ private final String tableName;
+ private final String location;
+ private final String inputFormat;
+ private final String serializationLib;
+ private final boolean firstColumnIsString;
+ private final List<String> partitionKeyNames;
+ private final List<HmsPartitionInfo> prunedPartitions;
+
+ private HiveScanReuseKey(long catalogId, String queryId,
HiveTableHandle handle,
+ ConnectorScanRequest request) {
+ // The catalog id and query id isolate same-named tables across a
cross-catalog
Review Comment:
**Issue: `ConnectorScanRequest request` parameter accepted but never used.**
All fields in the constructor are sourced from `handle` alone — the
`request` parameter is completely unused. This is inconsistent with Iceberg and
Paimon keys, which consume `request.getFilter()`, `request.getColumns()`, and
`request.isCountPushdown()`.
Two options: (1) remove the parameter if Hive scan-planning is truly
independent of the request's filter/columns (partition pruning already captures
the relevant variability), or (2) add a comment explicitly documenting why
`request` is deliberately excluded here — otherwise future readers may wonder
whether it was an oversight.
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java:
##########
@@ -2245,4 +2267,79 @@ private static String escapeJson(String s) {
private ConnectorStorageContext storage() {
return context.getStorageContext();
}
+
+ /**
+ * Statement-scoped cache key for one Paimon scan.
+ *
+ * <p>Includes every input that changes the planned split list: table
identity, the branch pin,
+ * the whole scan-options map (snapshot / tag / incremental / options
pins), the projected
+ * columns in order, the pushed filter and the COUNT pushdown flag. System
tables are excluded
+ * upstream, and session variables are statement-constant, so both stay
out of the key.
+ */
+ private static final class PaimonScanReuseKey {
+ private final long catalogId;
+ private final String queryId;
+ private final String databaseName;
+ private final String tableName;
+ private final String branchName;
+ private final Map<String, String> scanOptions;
+ private final List<String> columnNames;
+ private final List<ConnectorExpression> filterConjuncts;
+ private final boolean countPushdown;
+
+ private PaimonScanReuseKey(long catalogId, String queryId,
PaimonTableHandle handle,
+ ConnectorScanRequest request) {
+ // The catalog id and query id isolate same-named tables across a
cross-catalog
+ // statement and executions of a reused prepared statement (see
+ // ConnectorStatementScopes.resolveInStatement). System tables are
bypassed in planScan
+ // before this key is built, so sysTableName is always null here;
if the system-table
+ // bypass is ever relaxed, add it back.
+ this.catalogId = catalogId;
+ this.queryId = queryId;
+ this.databaseName = handle.getDatabaseName();
+ this.tableName = handle.getTableName();
+ this.branchName = handle.getBranchName();
+ this.scanOptions = handle.getScanOptions() == null
+ ? Collections.emptyMap()
+ : Collections.unmodifiableMap(new
HashMap<>(handle.getScanOptions()));
+ this.columnNames = request.getColumns().stream()
+ .filter(PaimonColumnHandle.class::isInstance)
+ .map(column -> ((PaimonColumnHandle)
column).getName().toLowerCase(Locale.ROOT))
Review Comment:
**Defensive-coding concern: silent column-type filtering.**
The `.filter(PaimonColumnHandle.class::isInstance)` silently drops any
column that is not a `PaimonColumnHandle`. While all columns in a Paimon scan
request should be of that type in practice, if a mixed-type column list ever
occurs (e.g., due to SPI evolution), two requests with different column sets
could produce identical keys.
Two alternatives: (1) Remove the filter and do a direct cast — if the
assumption is that all columns are `PaimonColumnHandle`, the ClassCastException
on a violation is a better failure mode than silent data loss. (2) Add an
explicit type-check that throws `IllegalArgumentException` for unexpected
column types, making the invariant explicit.
(The Iceberg key avoids this issue entirely by not including column names in
the key — is column projection in Paimon truly necessary for split-plan
identity?)
--
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]
