github-actions[bot] commented on code in PR #66530:
URL: https://github.com/apache/doris/pull/66530#discussion_r3749204773
##########
fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveScanPlanProvider.java:
##########
@@ -238,6 +260,20 @@ public List<ConnectorScanRange> planScanForPartitionBatch(
ConnectorSession session,
ConnectorScanRequest request,
List<String> partitionBatch) {
+ if (session == null) {
+ return doPlanScanForPartitionBatch(session, request,
partitionBatch);
+ }
+ HiveScanReuseKey reuseKey = new
HiveScanReuseKey(session.getCatalogId(), session.getQueryId(),
+ (HiveTableHandle) request.getTableHandle(), partitionBatch);
+ return session.getStatementScope().computeIfAbsent(reuseKey,
Review Comment:
**[P2] Close batch-only statement scopes after the split pump quiesces**
This batch memo is never deterministically released for deferred Arrow
Flight scans. Batch mode calls `startSplit` without calling `getSplits`, but
the query-finish `statementScope::closeAll` callback is registered only in
`getSplits`; Arrow Flight also sets `returnResultFromLocal=false`, so
`StatementContext.close` deliberately skips its fallback. Its finalizer
therefore unregisters the query with no scope callback, leaving these lists
(and other scoped closeables) until GC. Register cleanup for coordinated batch
scans too, and cancel/join the submitted batch futures before closing because
`SplitAssignment.stop()` currently does neither.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -418,8 +420,39 @@ public boolean supportsFileCache() {
*/
@Override
public List<ConnectorScanRange> planScan(ConnectorSession session,
ConnectorScanRequest request) {
- return planScanInternal(session, request.getTableHandle(),
request.getColumns(),
- request.getFilter(), request.isCountPushdown());
+ IcebergTableHandle icebergHandle = (IcebergTableHandle)
request.getTableHandle();
+ if (session == null) {
+ return planScanInternal(session, request.getTableHandle(),
request.getColumns(),
+ request.getFilter(), request.isCountPushdown());
+ }
+ if (icebergHandle.isSystemTable()) {
+ // System tables read connector metadata through their own
readers; never reuse them.
+ return planScanInternal(session, request.getTableHandle(),
request.getColumns(),
+ request.getFilter(), request.isCountPushdown());
+ }
+ // Statement-scoped reuse: within one statement the identical scan
(same table, same
+ // snapshot/ref/schema pin, same filter, same COUNT pushdown) plans
once and every
+ // duplicated relation shares the result. The scope is NONE for
offline planning and tests,
+ // in which case the loader runs on every call. Session variables are
constant within a
+ // statement and deliberately absent from the key.
+ //
+ // The ranges are memoized in a map HUNG INSIDE the statement scope
rather than cached
+ // directly: planScanInternal re-enters the scope itself (sharedTable
and the v3
+ // rewritableDeleteSupply are scope-backed), and a loader of the
scope's
+ // ConcurrentHashMap must not touch that map (same-bin re-entry throws
+ // IllegalStateException("Recursive update"); a mid-computation resize
silently drops the
+ // outer entry). The scope loader only constructs the memo map;
planning then runs on that
+ // separate map, so every scope call from planScanInternal is
top-level again. The memo
+ // key is catalog-scoped, which also isolates same-named tables across
a cross-catalog
+ // statement.
+ String memoKey = "iceberg.scan-reuse:" + session.getCatalogId() + ":"
+ session.getQueryId();
Review Comment:
**[P2] Reuse the Iceberg equality-delete preflight as well**
For every duplicate eager scan node, `getScanNodeProperties` independently
calls `hasApplicableEqualityDeletes(exactScan)`. When the snapshot summary is
positive or missing, that helper opens the exact filtered `scan.planFiles()`
and may traverse all selected tasks; this happens outside `scanReuse`, before
the later range plan reaches this memo. Thus equality-delete tables still
perform the expensive remote file-plan traversal once per alias even though
their ranges are reused. Please memoize applicability under the same
table/MVCC/filter/rewrite identity (or return it with the shared plan) and
cover two same-statement nodes with a `planFiles()` call-count test.
##########
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 {
Review Comment:
**[P1] Reject newly built plugins on older API-5 FEs**
The String bridge repairs old-plugin -> new-FE calls, but the reverse
direction still breaks under the unchanged major. Every newly built connector
invokes `computeIfAbsent(Object, Supplier)`, which a pre-PR API-5 FE does not
provide; Iceberg and Paimon additionally link this new helper. Because plugin
ZIPs exclude `fe-connector-spi` and that package is parent-first, the old FE
admits the plugin and then fails with `NoSuchMethodError` or
`NoClassDefFoundError` on its first reused scan. Please bump the connector API
major and extend the frozen surface so incompatible plugins are rejected at
load time.
--
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]
