[opt](filescan) Reuse external scan tasks within a statement - #66530
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
141fd94 to
cc165ac
Compare
|
run buildall |
495ee54 to
ab618f7
Compare
|
run buildall |
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 29000 ms |
TPC-DS: Total hot run time: 166728 ms |
ClickBench: Total hot run time: 23.96 s |
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
ab618f7 to
02b8284
Compare
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
02b8284 to
a008699
Compare
|
run buildall |
TPC-H: Total hot run time: 28767 ms |
### What problem does this PR solve? Issue Number: None Related PR: apache#66530 Problem Summary: Statement-scoped connector scan reuse is valuable for repeated external-table aliases but needs an immediate incident escape hatch. The connector SPI receives the default-on switch as an existing string session property and Hive, Hudi, Iceberg, and Paimon bypass their statement memo when it is false. The SPI remains independent of fe-core types. ### Release note The new session variable enable_external_scan_task_reuse defaults to true. Set it to false to disable statement-scoped external split reuse. ### Check List (For Author) - Test: Unit Test - run-fe-ut.sh with fe-connector-hudi HudiScanReuseKeyTest - Behavior changed: Yes (a session switch can bypass external split reuse) - Does this need documentation: No
Issue Number: None Related PR: apache#66530 Problem Summary: Scan-reuse memos used inline statement-scope namespaces, and Hive used `hive` instead of its connector type `hms`. Since the statement scope is a shared untyped registry, unguarded namespace drift can collide with another owner and return an incompatible memo value. Declare connector-owned namespace constants for Hive, Hudi, Iceberg, and Paimon, and test every prefix against its provider type. None - Test: Unit Test - HiveScanBatchModeTest - HudiScanReuseKeyTest - IcebergScanPlanProviderTest - PaimonScanPlanProviderTest - ./build.sh --fe - Behavior changed: No - Does this need documentation: No
Problem Summary: Mixed-version connector sessions could enable scan-task reuse without an explicit gate, partition batch admission could starve its bounded executor, and split-source shutdown could race final resource cleanup with an active producer. This change requires an explicit true reuse gate, keeps partition planning in one tracked asynchronous producer with queue backpressure, and separates thread-safe cancellation from producer-owned close. Release note: None Test Plan: - SplitAssignmentTest and PluginDrivenScanNodeBatchModeTest - HiveScanBatchModeTest, HudiScanReuseKeyTest, IcebergScanPlanProviderTest, and PaimonScanPlanProviderTest - mvn checkstyle:check -pl fe-core - ./build.sh --fe
Problem Summary: Recent review fixes expanded this scan-reuse PR into partition-batch scheduling, streaming split cancellation, generic query callback ordering, and MTMV execution lifecycle changes. Remove those unrelated changes. Keep only non-batch statement-scoped scan reuse, its explicit mixed-version gate, connector-specific exclusions, SPI compatibility coverage, and the minimal background-task scope cleanup needed to release retained reuse results. Release note: None Test Plan: - Connector reuse and SPI focused unit tests - TaskProcessorTest - FE Checkstyle - ./build.sh --fe
Issue Number: None Related PR: apache#66530 Problem Summary: Hive, Hudi, Iceberg, and Paimon each parsed the same enable_external_scan_task_reuse session property, which duplicated the engine-owned policy and allowed future connectors to diverge. Expose one fail-closed ConnectorSession method and route all four providers through it. Preserve Iceberg metadata-column reader validation before a statement-reuse cache hit can return ranges planned for ordinary columns. Because current master already serves connector plugin API 7.0, advance the major to 8.0 for the new frozen ConnectorSession method. None - Test: Unit Test - ConnectorPluginSurfaceTest - ConnectorMetadataSurfaceTest - ConnectorSessionImplTest - HiveScanBatchModeTest - HudiScanReuseKeyTest - IcebergScanPlanProviderTest - PaimonScanPlanProviderTest - TaskProcessorTest - ./build.sh --fe - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66530 Problem Summary: Paimon and Hudi scan reuse could compute a generation token from one physical table instance, plan file ranges from a newly resolved instance, and serialize scan properties from another instance. A same-path table recreation or schema change could therefore pair generation-A ranges with generation-B schema metadata. Resolve one statement-scoped physical table holder per complete data-table relation identity and pass that exact object through planning and property construction. Keep Paimon system-table wrapper and source state handle-local because system-table split planning bypasses reuse; otherwise equal aliases can combine one alias wrapper with another alias source and ranges. Include Paimon branch and effective scan options plus the Hudi base path in resolution identities. Authenticate the Paimon latest-snapshot lookup because it may read snapshot metadata through a Kerberos-backed FileIO after table resolution has left its authenticated scope. Advance the connector plugin API to 10.0 because ConnectorSession adds the external scan reuse policy method. ### Release note Fix external Paimon and Hudi scan reuse across table generations. ### Check List (For Author) - Test: Unit Test and FE build - PaimonScanPlanProviderTest: 100 tests passed - HudiScanReuseKeyTest - ConnectorPluginSurfaceTest - FE Maven reactor in ./build.sh --fe --clean -j2: BUILD SUCCESS; generated output packaging then stopped because an existing jdbc.conf makes macOS cp -n return 1 - Behavior changed: Yes, reused scans now stay on one physical table generation within a statement - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66530 Problem Summary: Hive directory-listing failures and Hudi schema-resolution fallbacks can return usable but degraded scan plans. Statement-level scan reuse previously cached those results, preventing a later alias in the same statement from retrying after a transient failure. Track plan completeness and return degraded results without installing them in the statement memo, while preserving the existing tolerant behavior. Add fail-once/succeed-next tests for Hive listing and Hudi per-file schema resolution. ### Release note None ### Check List (For Author) - Test: Unit Test - HiveScanBatchModeTest (25 tests passed) - HudiScanReuseKeyTest (14 tests passed) - Hive/Hudi FE connector Checkstyle - Behavior changed: Yes; degraded scan plans are no longer reused within a statement - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static exact-head review of 98f451dd4fa8afb68e98d0e43f2019de2c82001b.
Outcome: request changes for one distinct P2 performance defect in Paimon statement-scoped scan reuse. No other P1/P2 survived the existing-thread duplicate fence and two bounded full-review/risk-review rounds.
Critical checkpoint conclusions:
- Goal, scope, and tests: the PR consistently adds statement-scoped range-plan reuse across Hive, Hudi, Iceberg, and Paimon, with the necessary SPI, session-variable, and lifecycle plumbing. The targeted tests cover key equality, reuse hits, degraded-plan rejection, generation coupling, and scope cleanup, but the Paimon test currently codifies two generation probes for two equal requests and does not cover live-latest churn or the pinned-empty no-probe path.
- Correctness, error handling, and concurrency: reuse keys were traced against the inputs actually consumed by each planner. Tolerated Hive/Hudi degradation remains caller-local and is not cached; memoized values are immutable and copied before downstream sampling. No distinct wrong-result, publication, or mutation issue remains.
- Lifecycle and ownership: foreground query callbacks, worker cleanup, MTMV child scopes, retry/IVM paths, and statement-scope teardown were traced. The changed head has deterministic owned cleanup; the remaining suspicious lifecycle shapes are already covered by existing inline threads.
- Configuration and compatibility: the forwarded
enable_external_scan_task_reusevalue, fail-closed SPI default, connector API-major bump, and frozen surface are internally consistent. No separate compatibility issue remains. - Parallel paths and performance: Hive batch, Iceberg streaming, transactional, and system-table exclusions preserve their bounded-memory or semantic contracts. The remaining Paimon issue is that fixed/pinned aliases still perform a live remote generation read before the memo lookup, and a concurrent latest-pointer advance can force a redundant retained plan for the same pinned scan. Open-ended incremental/dynamic selectors are explicitly outside this finding.
- Persistence, protocol, and observability: this change introduces no EditLog/storage-format/data-write change or new FE-BE payload. Existing profiles/logging remain adequate for the changed behavior.
User focus: no additional focus was supplied; the complete changed-file set and related call chains were reviewed. Validation status: static review only, as this review environment prohibited builds and tests; test conclusions above are based on source and diff inspection, not independent execution.
| // this, a no-cache catalog or a schema change between two aliases can give alias A's ranges | ||
| // to alias B's generation-B serialized table. | ||
| Table table = resolveScanTableConsistent(session, paimonHandle); | ||
| long generation = resolvePaimonGeneration(table); |
There was a problem hiding this comment.
[P2] Avoid re-reading live latest for already-fixed scan identities. Normal/latest, snapshot/timestamp, branch, statement-fenced options, and pinned-empty reads reach this method with their scan identity fixed on the handle, but this probe runs before scanReuse.computeIfAbsent. N equal aliases therefore still perform N authenticated snapshot-pointer reads (the changed test explicitly expects two); if latest advances from S1 to S2 between aliases, the generation field also forces another full retained plan even though both requests still scan pinned S1. Derive or memoize the generation for fixed handles, and keep live fencing or bypass reuse only for genuinely latest-dependent selectors such as open-ended @incr. Please cover both cases, plus a pinned-empty path that does not probe latest.
There was a problem hiding this comment.
Fixed in 616b309. Fixed snapshot identities now use their pinned snapshot id as the reuse generation, and pinned-empty identities skip the live latest lookup entirely. Latest-dependent selectors such as open-ended incremental scans still read and fence on the live generation. Added coverage for all three paths; PaimonScanPlanProviderTest passes 103/103 and ./build.sh --fe passes.
TPC-H: Total hot run time: 27831 ms |
TPC-DS: Total hot run time: 151575 ms |
ClickBench: Total hot run time: 23.94 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#66530 Problem Summary: Paimon scans whose handles were already pinned to a snapshot, including pinned-empty scans, still read the live latest snapshot before consulting statement-level scan reuse. Equal aliases therefore repeated authenticated metadata I/O, and a concurrent commit could assign different generation keys to aliases that both scan the same pinned snapshot. Use the pinned snapshot as the generation for fixed identities and skip the live probe for pinned-empty identities, while retaining live generation fencing for selectors such as open-ended incremental scans. ### Release note None ### Check List (For Author) - Test: Unit Test - PaimonScanPlanProviderTest (103 tests passed) - ./build.sh --fe - Behavior changed: Yes; fixed Paimon scan identities no longer read the live latest snapshot for reuse-key generation - Does this need documentation: No
|
/review |
|
run buildall |
TPC-H: Total hot run time: 27828 ms |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-DS: Total hot run time: 152562 ms |
ClickBench: Total hot run time: 23.83 s |
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Static review result: no new non-duplicate actionable findings were found on requested head 616b309de361b562641ee8f8e5d8e1507a3a446e. I did not repost concerns already covered by the live inline threads; this COMMENT does not resolve or supersede those discussions.
Critical checkpoint conclusions:
- Goal and proof: the change reuses equivalent non-batch external scan range plans within one statement for Hive, Hudi, Iceberg, and Paimon. The new tests exercise live-scope hits, structural filters, disabled/missing settings, transient retry behavior, generation pairing, and cleanup, although this review did not execute them.
- Scope and focus: the 21 changed files are focused on connector plan reuse, its SPI/configuration surface, statement/task cleanup, and targeted tests. No additional user focus was supplied.
- Concurrency: connector-private
ConcurrentHashMapmemos provide same-key single-flight behavior; returned lists/ranges are immutable and engine scan nodes create fresh mutableSplitwrappers. Session-derived routing inputs are statement-constant. Existing lifecycle discussions were treated as duplicate fences. - Lifecycle: memo ownership follows
StatementContextand query-finish cleanup; task contexts unregister query callbacks and close statement resources. I found no new lifecycle mechanism beyond concerns already present in the inline review. - Configuration:
enable_external_scan_task_reusedefaults on in FE, is forwarded to the planning FE, and the SPI default enables reuse only when the session property is explicitly present and true. - Compatibility: the new
ConnectorSessionsurface is frozen in the API baseline and the connector plugin API major moves from 9 to 10, matching the exact-major gate. No new FE/BE wire or storage-format field is introduced. - Parallel paths and special cases: Hive partition-batch, Iceberg streaming, Hive transactional scans, and connector system-table scans remain deliberately uncached where reuse would violate resource or side-effect semantics. Iceberg keeps per-call metadata-reader validation; Hive/Hudi incomplete results remain retryable.
- Tests and results: the changed tests target the principal key, generation, retry, and cleanup contracts. Per the review instructions, no build or test command was run, so author/CI results were not independently validated.
- Observability: existing scan profiles and contextual warning/info logs cover planning and fallback paths; this statement-local optimization did not expose a distinct missing diagnostic that warrants a review finding.
- Transactions and persistence: no EditLog, replay, durable metadata, or transaction-commit format is changed.
- Data writes: no data-write algorithm or atomicity contract is changed; the reviewed behavior is FE-side read planning and resource cleanup.
- FE/BE variables: the new setting remains an FE planning variable and is forwarded between FEs; existing scan-range/property carriers to BE are reused unchanged.
- Performance: normal-path duplicate planning and remote metadata work are eliminated while large Hive batch and Iceberg streaming paths stay uncached to preserve bounded memory. Statement-held plans are released with the statement; no new substantiated CPU or memory regression was found.
- Other/error handling: Hive skipped-directory and Hudi schema-degradation results avoid cache installation so later aliases can retry. Hudi's incremental full-table fallback is an intentional complete alternate plan with the fixed row predicate, not a partial result.
Validation was static only, as required.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Queries with repeated equivalent external-table relations independently plan the same remote files for every branch, duplicating connector metadata work. The connector SPI already provides statement-scoped memoization for metadata, but scan planning was not memoized, so each duplicate relation re-planned through the connector SDK.
This change adds statement-scoped scan reuse to all four connectors. Each builds a typed key from the facts that affect its split set:
Equivalent non-batch scans reuse connector-native tasks within one statement execution. Iceberg streaming/lazy batch paths, Hive partition-batch paths, transactional Hive scans, and system-table paths remain uncached to preserve their existing memory, transaction, and planning semantics.
The public ConnectorStatementScope String-key SPI remains unchanged for plugin compatibility. Each connector stores a private typed-key map under its own statement-scope namespace. The reuse switch is forwarded with the session and must be explicitly true, so an older planning FE that omits it safely disables reuse. Background task workers close their final StatementContext after each task so retained scan plans do not remain reachable for the worker lifetime.
Release note
Reuse equivalent non-batch Iceberg, Paimon, Hudi, and Hive scan-planning results within one statement execution.
Check List (For Author)