github-actions[bot] commented on code in PR #66530:
URL: https://github.com/apache/doris/pull/66530#discussion_r3746210972
##########
fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorStatementScope.java:
##########
@@ -42,7 +42,7 @@ public interface ConnectorStatementScope {
* caching it for the rest of the statement. Within one statement the same
key returns the same
* instance to every caller; under {@link #NONE} the loader runs on every
call.
*/
- <T> T computeIfAbsent(String key, Supplier<T> loader);
+ <T> T computeIfAbsent(Object key, Supplier<T> loader);
Review Comment:
**[P1] Keep API-5 plugins from linking against a removed method descriptor**
This re-signs the SPI method from `(String, Supplier)` to `(Object,
Supplier)` (and does the same to `getOrCreateMetadata`). A connector already
compiled against API 5.0 still issues the old `invokeinterface` descriptor, but
the unchanged major-version gate admits it because this FE also advertises 5.0;
its first statement-scope call then fails with `NoSuchMethodError` instead of
being rejected at load time. The documented connector contract classifies every
shared-SPI surface change as MAJOR, and the current surface test does not
freeze `ConnectorStatementScope`, so the green source tests cannot catch this.
Please either retain the API-5 descriptors behind a compatible object-key API
or bump the connector API major and refresh/extend the recorded surface so old
plugins are rejected deterministically.
##########
fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveScanPlanProvider.java:
##########
@@ -131,6 +133,26 @@ 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.
+ HiveScanReuseKey reuseKey = new
HiveScanReuseKey(session.getCatalogId(), session.getQueryId(),
+ hiveHandle);
+ return session.getStatementScope().computeIfAbsent(reuseKey,
Review Comment:
**[P2] Cover the Hive batch path in the reuse contract**
This memo only wraps synchronous `planScan`. For a partitioned
non-transactional table whose selected partition count reaches
`num_partitions_in_batch_mode`, `PluginDrivenScanNode.startSplit` calls Hive's
`planScanForPartitionBatch` directly; that override repeats batch resolution,
conversion, and split construction without touching this scope entry (and
repeats the underlying HMS/listing work whenever those caches are disabled,
cold, or evicted). Thus the repeated-relation case this PR targets still
bypasses reuse on large Hive scans. Please either document and test this path
as an intentional exclusion, or use a genuinely bounded sharing design;
retaining every completed batch list in the statement scope until `closeAll`
would accumulate the whole scan and defeat batch mode's memory bound.
##########
fe/fe-connector/fe-connector-hudi/src/test/java/org/apache/doris/connector/hudi/HudiScanReuseKeyTest.java:
##########
@@ -0,0 +1,114 @@
+// 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.hudi;
+
+
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+
+/** Statement-scoped scan reuse key construction for Hudi (offline; no table
environment needed). */
+class HudiScanReuseKeyTest {
+
+ private static HudiTableHandle handle() {
+ return new HudiTableHandle.Builder("db", "t", "/warehouse/t",
"COPY_ON_WRITE")
+ .inputFormat("org.apache.hudi.hadoop.HoodieParquetInputFormat")
+ .partitionKeyNames(Arrays.asList("year", "month"))
+ .prunedPartitionPaths(Arrays.asList("year=2025/month=01",
"year=2025/month=02"))
+ .queryInstant("20250429000000000")
+ .build();
+ }
+
+ private static HudiScanPlanProvider.HudiScanReuseKey key(HudiTableHandle
handle) {
+ return HudiScanPlanProvider.hudiScanReuseKey(0, "q", handle);
+ }
+
+ @Test
+ void sameScanYieldsSameKey() {
+ Assertions.assertEquals(key(handle()), key(handle()),
+ "two identical handles must produce the same reuse key");
+ }
+
+ @Test
+ void differentCatalogIdYieldsDifferentKey() {
+ HudiScanPlanProvider.HudiScanReuseKey otherCatalog =
HudiScanPlanProvider.hudiScanReuseKey(1, "q", handle());
+ Assertions.assertNotEquals(key(handle()), otherCatalog,
+ "same-named tables in different catalogs must not reuse the
cached ranges");
+ }
+
+ @Test
+ void differentQueryIdYieldsDifferentKey() {
+ HudiScanPlanProvider.HudiScanReuseKey otherQuery =
HudiScanPlanProvider.hudiScanReuseKey(0, "q2", handle());
+ Assertions.assertNotEquals(key(handle()), otherQuery,
+ "a different query id must not reuse the cached ranges");
+ }
+
+ @Test
+ void differentQueryInstantYieldsDifferentKey() {
Review Comment:
**[P2] Make each reuse-key test isolate the discriminator it names**
This case compares the fully populated `handle()` fixture with a new handle
that also drops the input format, partition-key names, and pruned partitions.
Removing `queryInstant` from `HudiScanReuseKey` would therefore still leave
these keys unequal and this test green; the pruned-partition case has the same
multi-field problem. Please derive the variant from `handle().toBuilder()` and
change exactly one field. More broadly, this is the only new key test in the
diff, and no changed test calls any provider twice under a live scope to prove
one underlying plan (or exercises the Hive batch path), so the advertised
four-connector reuse and its concurrency contract currently have no behavioral
regression coverage.
--
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]
