morrySnow commented on code in PR #66530:
URL: https://github.com/apache/doris/pull/66530#discussion_r3773488732
##########
fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java:
##########
@@ -2246,4 +2269,76 @@ 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 String databaseName;
+ private final String tableName;
+ private final String branchName;
+ private final Map<String, String> scanOptions;
+ private final List<String> columnNames;
+ private final Optional<ConnectorExpression> filter;
+ private final boolean countPushdown;
+
+ private PaimonScanReuseKey(PaimonTableHandle handle,
ConnectorScanRequest request) {
+ // Catalog and query isolation are provided by the statement-scope
memo key. 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.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()
+ .map(PaimonScanReuseKey::toPaimonColumnName)
+ .collect(Collectors.toList());
+ this.filter = request.getFilter();
Review Comment:
Same issue as the Iceberg key: `ConnectorExpression` has no
`equals`/`hashCode`, so `Optional<ConnectorExpression>` equality here is
identity-based. Two duplicate relations with an equal pushed filter (each scan
node builds its own expression tree) will produce different keys, so the reuse
misses for filtered scans — which for Paimon is the common case since partition
pruning is predicate-driven — and each distinct instance's plan is retained in
the statement scope until close. Since the filter participates in `scan.plan()`
split enumeration, it must stay in the key, but the comparison needs to be
structural (or canonicalized) for the memo to actually hit.
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanPlanProvider.java:
##########
@@ -2922,4 +2953,70 @@ private Table resolveSysTable(ConnectorSession session,
IcebergTableHandle handl
private ConnectorStorageContext storage() {
return context.getStorageContext();
}
+
+ /**
+ * Statement-scoped cache key for one Iceberg scan.
+ *
+ * <p>Includes every input that changes the planned file list: table
identity, the typed
+ * time-travel pin (snapshot id / ref / schema id), the rewrite-file
scope, 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 IcebergScanReuseKey {
+ private final String dbName;
+ private final String tableName;
+ private final long snapshotId;
+ private final String ref;
+ private final long schemaId;
+ private final List<String> rewriteFileScope;
+ private final Optional<ConnectorExpression> filter;
+ private final boolean countPushdown;
+
+ private IcebergScanReuseKey(IcebergTableHandle handle,
ConnectorScanRequest request) {
+ this.dbName = handle.getDbName();
+ this.tableName = handle.getTableName();
+ this.snapshotId = handle.getSnapshotId();
+ this.ref = handle.getRef();
+ this.schemaId = handle.getSchemaId();
+ // 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.rewriteFileScope = handle.getRewriteFileScope() == null
+ ? Collections.emptyList()
+ : handle.getRewriteFileScope().stream().sorted()
+ .collect(Collectors.toList());
+ this.filter = request.getFilter();
Review Comment:
`ConnectorExpression` (spi/pushdown) does not override `equals`/`hashCode`
(it only declares `getChildren()`), so `Optional<ConnectorExpression>` equality
in this key is reference-based, not structural. Each scan node builds its own
filter tree via `PluginDrivenScanNode.buildRemainingFilter()`, so two duplicate
relations carrying the same pushed filter almost never produce equal keys:
1. The memo misses for exactly the filtered case this PR targets (e.g.
self-join with a common predicate; Paimon's filter-driven partition pruning).
Reuse only works when the filter is empty or the literally same instance is
passed — the new unit tests pass the same `request` object twice, so they
cannot catch this.
2. Each distinct filter instance's full range list is then retained in the
statement scope until statement end, where previously it was released after
`getSplits` — a memory regression for statements with many distinct filtered
scans of the same table.
This is conservative (no wrong reuse), but the reuse silently does not
happen for filtered scans. Consider giving `ConnectorExpression` structural
equality (or canonicalizing the filter in the key), or at least documenting the
limitation and its memory consequence.
##########
fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveScanPlanProvider.java:
##########
@@ -725,4 +759,72 @@ 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 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(HiveTableHandle handle) {
+ // Catalog and query isolation are provided by the statement-scope
memo key. The table
+ // location identifies the data source of unpartitioned tables,
whose prunedPartitions
+ // is null.
+ this.dbName = handle.getDbName();
+ this.tableName = handle.getTableName();
+ this.location = handle.getLocation();
+ this.inputFormat = handle.getInputFormat();
+ this.serializationLib = handle.getSerializationLib();
+ this.firstColumnIsString = handle.isFirstColumnString();
+ this.partitionKeyNames = handle.getPartitionKeyNames() == null
+ ? Collections.emptyList()
+ : Collections.unmodifiableList(new
ArrayList<>(handle.getPartitionKeyNames()));
+ this.prunedPartitions = handle.getPrunedPartitions() == null
+ ? null
+ : Collections.unmodifiableList(new
ArrayList<>(handle.getPrunedPartitions()));
Review Comment:
Two low-severity notes on the key construction:
1. The defensive `new ArrayList<>(...)` copy plus `Objects.hash(...)` over
the entire pruned partition list runs on every `planScan` call (including memo
hits, since the key is built before lookup). For tables with very large pruned
sets (up to `MAX_PARTITIONS` = 100000) each duplicated relation pays an
O(partitions) copy and hash per call.
2. `HmsPartitionInfo.equals`/`hashCode` cover only `values` + `location`,
but `convertPartitions` derives the per-partition format (`splittable`, and
thus the split list) from `part.getInputFormat()`/`getSerializationLib()`,
which are not in the equality. In practice the same HMS partition identity
implies the same format, so this is unlikely to produce a wrong reuse —
flagging only so the key's javadoc claim of covering the file formats stays
accurate.
##########
fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveScanPlanProvider.java:
##########
@@ -131,6 +133,28 @@ public boolean usesHiveParquetInt96TimeZone() {
@Override
public List<ConnectorScanRange> planScan(ConnectorSession session,
ConnectorScanRequest request) {
+ HiveTableHandle hiveHandle = (HiveTableHandle)
request.getTableHandle();
+ if (session == null) {
+ return doPlanScan(session, request);
+ }
+ if (hiveHandle.isTransactional()) {
+ // ACID / INSERT_ONLY reads open a per-scan read transaction with
a write-id snapshot and
+ // a shared metastore lock; reusing the planned ranges would skip
that transaction.
+ return doPlanScan(session, request);
+ }
+ // Statement-scoped reuse: within one statement the identical scan
(same table, same
+ // partition set, same formats) 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.
+ String memoKey = "hive.scan-reuse:" + session.getCatalogId() + ":" +
session.getQueryId();
+ Map<HiveScanReuseKey, List<ConnectorScanRange>> scanReuse =
session.getStatementScope().computeIfAbsent(
+ memoKey, () -> new ConcurrentHashMap<>());
+ HiveScanReuseKey reuseKey = new HiveScanReuseKey(hiveHandle);
+ return scanReuse.computeIfAbsent(reuseKey,
+ key -> Collections.unmodifiableList(doPlanScan(session,
request)));
Review Comment:
Retention note: every distinct (unreused) scan now pins its full range list
inside the statement scope until `closeAll()` at query finish, instead of being
released after each node's `getSplits`. For a statement with many distinct
external scans this is newly retained memory. For Hive specifically, the
large-scan path (partition-batch mode, `planScanForPartitionBatch`) is
deliberately not memoized, so on that path the memo only ever serves
planning/EXPLAIN calls while execution streams batches — the memoized full plan
is retained for the whole statement without ever being reused. Worth confirming
this is acceptable; if not, the `planScan` memo could be skipped when the table
qualifies for batch scan.
--
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]
