Repository navigation
[improvement](fe) Default external scans to bounded split batches - #69683
Merged
924060929 merged 9 commits intoOct 29, 2026
Merged
Conversation
### What problem does this PR solve? Issue Number: None Problem Summary: External scans previously entered batch mode only above 1024 partitions or files. When enabled, each producer batch held 128 splits and each backend queue was bounded by 32 batches, so small LIMIT queries could wait for unnecessary work while the queue capacity depended on batch size. Enable batch mode from one partition or file, separate the Hive partition-processing size from that trigger, and store individual splits in a queue bounded by 1024 splits per backend. Producers still offer splits in batches; use eight splits per batch by default. In a matched one-BE Release benchmark on the integrated test branch, refreshed-metadata LIMIT medians at batch sizes 8 versus 32 were 75.8 versus 102.5 ms for Hive and 23.5 versus 30.6 ms for Iceberg. Warm full scans showed no stable winner across those sizes. Preserve request batch limits, cancellation, backpressure, and producer wakeups. The new connector SPI method advances the plugin API major version to 25. ### Release note External table Batch Mode now starts at one partition or file by default. The default producer batch size is eight splits, queue capacity is 1024 splits per backend, and split_partition_batch_size controls Hive partition-processing batches separately. ### Check List (For Author) - Test: Unit Test. Before rebasing onto current master, FE build and ten focused FE test classes passed (162 tests). On current master, ConnectorPluginSurfaceTest (2), ConnectorMetadataSurfaceTest (4), and HiveScanBatchModeTest (36) passed. FE-core Maven tests could not start locally because snapshot reactor artifacts were unresolved without -am; the full -am route requires an x86 grpc generator unavailable on this ARM host. One-BE Release benchmark results above were recorded on the integrated test branch. - Behavior changed: Yes (Batch Mode thresholds and split buffering defaults change as described above) - Does this need documentation: No (connector scheduling document updated in this commit)
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Static review of PR #69683 at 5a8470c found one P2 regression: an awaited one-split fetch can return OK despite a producer failure already being published. The inline comment contains the triggering sequence and fix direction. There are no existing inline findings to carry forward and no additional user-provided focus points.
Critical checkpoint conclusions:
- Goal and evidence: defaults and split-level buffering implement the intended bounded batching; inspected tests cover partial offers, sizing and sync/streaming parity. Error preservation has the inline exception below.
- Scope and reuse: changes stay within scan scheduling, Hive request sizing, session settings, SPI versioning, documentation and their tests; existing producer adapters and scheduler are reused.
- Concurrency: split workers, FE fetch RPC threads and query cleanup share queue state under per-backend monitors; offsets retain partial offers. Callbacks run after releasing queue/listener locks, with registration/retry preventing capacity wakeup loss. No additional deadlock found; the awaited-fetch terminal check is the reported gap.
- Lifecycle: producers retain bounded pending batches, query cleanup signals waiters and closes sources cooperatively, and completion removes registrations. No new static initialization dependency found.
- Configuration: split_partition_batch_size is positive-validated, session/global configurable through existing variable machinery, forwarded between FEs and exposed to connectors without restart. Existing batch/queue settings now bound split counts as documented.
- Compatibility: the added SPI method is recorded and the connector API major advances to 25.0. Major-version gating remains in place; stored data formats and FE/BE RPC fields are unchanged.
- Thrift/protobuf: no generated definitions change; cross-branch field-ID comparison is inapplicable.
- Parallel paths: checked partition batching, file streaming and legacy addToQueue, Hive/MaxCompute partition scoping and Iceberg streaming exclusions; other connectors retain their opt-in gates.
- Conditions: sampling, empty partitions, missing slots, deferred Variant compatibility, Iceberg metadata COUNT and v3 exclusions retain their guards. Queue positivity and offset preconditions have concrete invariants.
- Tests: inspected unit coverage for partial acceptance, unique ranges, backpressure, wakeup registration, completion/failure/cancellation, first-fetch bounds, partition concurrency and invalid settings, plus regression sync/streaming comparisons. The awaited one-split/error interleaving needs coverage.
- Expected results: no expected-result files change; the regression edit adds setting save/restore and partition size only. No new result content needs validation.
- Observability: existing source/query IDs and failure logging remain; the generation profile now reports the effective clamped batch size. No additional instrumentation gap established.
- Transactions/persistence: no EditLog, replay, failover or transaction state changes; those checkpoints are inapplicable.
- Data writes/atomicity: no data-write behavior changes. Scan failure propagation is covered by the inline finding; no separate crash-related ownership issue established.
- FE/BE variable passing: the new setting controls FE planning only; existing session forwarding and split-source transport are sufficient.
- Performance and further review: queue storage is bounded per split, producer batches are capped by queue capacity, drains amortize locking, and Hive keeps bounded listing lanes. No additional concrete performance or correctness defect was established by the final source review.
All 22 changed paths and related producer, consumer, connector and cleanup paths were reviewed. Validation was static only: no builds, tests, benchmarks or runtime reproductions were run, and the PR's reported performance numbers were not independently verified. No unresolved source-review candidates or uncovered assigned scope remain.
Review converged after 2 round(s).
| if (split != null) { | ||
| splitAssignment.notifyQueueSpace(backend); | ||
| pendingSplits = batch.iterator(); | ||
| scanRanges.add(split); |
Contributor
There was a problem hiding this comment.
[P2] Recheck terminal state after an awaited split
With split_initial_batch_size=1 (or a BE request size of 1), an empty-queue fetch waits in awaitSplit. A producer can enqueue a range and then publish a listing failure before the waiting RPC thread resumes. awaitSplit still polls that range, and adding it here fills the request, so the loop exits without another getAssignedSplits/isStop check. fetchSplitBatch consequently returns OK for an already-failed assignment. A LIMIT can consume that range and stop without another fetch; cleanup only logs the stop exception. Previously, the awaited batch remained pending until the loop rechecked terminal state. Apply the same error/cancellation recheck used by the bulk-drain branch after awaiting/notifying, and cover a one-split wait racing with producer failure.
Contributor
Author
There was a problem hiding this comment.
Fixed in 1145d3d. After an awaited split wakes and notifies queue-space listeners, the fetch now rechecks the assignment error and stop state before adding that split to the response. A deterministic one-split race test reproduced the old incorrect OK result for both producer failure and cancellation; both now pass. The complete SplitSourceTest class (33 tests) also passes via the existing Surefire classpath.
### What problem does this PR solve? Issue Number: None Problem Summary: A one-split BE fetch can wait for a range, then receive that range after its producer has already published a failure or cancellation. The new per-split queue path appended the awaited range and returned as soon as the request filled, without rechecking terminal state. Recheck after notifying queue space so an already-failed assignment returns its error and a stopped assignment discards the range. A deterministic two-case test reproduced the previous incorrect return and passes with the fix. ### Release note None ### Check List (For Author) - Test: Unit Test. The new race test failed in both cases before the fix, then passed in both cases afterward. All 33 SplitSourceTest cases passed using the prior Surefire classpath and JUnit launcher. The local FE-core Maven route is blocked by unresolved snapshot reactor artifacts; the full reactor needs an x86 gRPC generator unavailable on this ARM host. - Behavior changed: Yes (an already-failed or cancelled one-split fetch no longer returns OK with a range) - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 22 changed files at head 1145d3da7bef2159379d59713f25be67ba8dc38b. No new actionable findings. Review converged in 1 round: both complete-review agents and the additional partial-offer/terminal-race reviewer returned NO_NEW_VALUABLE_FINDINGS; all candidates and initial risks received evidence-backed dispositions.
The existing P2 awaited-split thread is fixed on this head. After an awaited split, SplitSource now rechecks the assignment exception and stop state before adding the range, including a one-split request. The added deterministic test covers failure and cancellation at that boundary. No duplicate comment is posted, and no existing P0/P1 blocker remains.
Critical checkpoint conclusions:
- Goal and proof: the implementation separates Hive partition work size from batch eligibility, bounds queues by individual splits, and retains partial offers. Unit tests target those mechanisms; their source supports the intended behavior, but they were not executed during this review.
- Scope and reuse: changes are focused on scan scheduling, configuration, SPI compatibility, tests and documentation; they reuse the existing producer, scheduler and cleanup framework.
- Concurrency and locking: traced FE dispatch -> split producers -> backend assignment/conversion -> per-BE queue -> split-fetch RPC -> BE scanners. Queue contents and waiter counts share a per-BE monitor; backend policy and producer scheduling retain their separate locks. Listener registration retries close the capacity-release race, callbacks run outside queue/listener monitors, and external listing/conversion stays outside queue locks. No introduced deadlock or lost wakeup was substantiated.
- Lifecycle and failure: retained offsets survive suspension without reconversion or replay. Finalization waits for dispatched producers, publishes failures before completion, and closes scoped sources. Stop wakes consumers before resource closure and unregisters sources. RPC errors remain non-OK statuses; finished queues drain before EOF. No introduced lifecycle leak or suppressed failure was found. No static-initialization change applies.
- Configuration:
split_partition_batch_sizehas a positive-value checker, default 16, generic session export and forwarding. Settings are captured for each statement without restart. Effective producer batches usemin(split_batch_size, split_queue_capacity); defaults and documentation agree on 8 and 1024 splits. - Compatibility: the new default SPI method, recorded plugin surface and API version assertion agree with connector API major 25. The existing version gate rejects mismatched plugins. No storage-format or wire-format change was introduced.
- Thrift/protobuf IDs: no
gensrc/thriftorgensrc/protochanges; cross-branch field-ID checks do not apply. - Parallel paths: checked cooperative and blocking producers, partition-batch Hive/MaxCompute, whole-scan Hive/Iceberg, and synchronous opt-outs. Required partition subsets remain scoped; TABLESAMPLE and deferred Variant checks retain their synchronous guards; servable Iceberg metadata counts remain synchronous. BE batch scanner concurrency is not capped by the small partition-count estimate.
- Conditional boundaries: positive batch eligibility excludes empty partition selections, and Hive's batch cap preserves independent listing lanes. Partial/full queues, first/subsequent fetches, empty batches, completion, failure and cancellation were checked without finding a distinct introduced defect.
- Test coverage: inspected partial-offer exact-once assertions, capacity-registration and terminal races, multi-BE streaming, first-fetch limits, empty/partial partition batches, session validation, SPI baselines and regression queries for pruning, LIMIT, joins, UNION and CTAS. The added tests exercise the changed behavior and negative boundaries.
- Test results: no
.outfile changes. New expected counts, range sets and metadata batch sizes agree with the fixtures; runtime results and prior result-file generation were not verified. - Observability: existing generated-split counters, queue-capacity information and BE fetch timing remain; the generation profile now reports the effective producer batch size. No additional observability defect was identified.
- Transaction and persistence: no edit-log, transaction or persisted-state change. The modified read path retains pinned snapshot/rewrite handles; transaction visibility, partition versions and MoW delete bitmaps are not modified.
- Writes, atomicity and crashes: no new data-write protocol. Review of cancellation and cleanup found no newly introduced resource-retention or deadlock path; crash recovery behavior is unchanged by this diff.
- FE-BE variables: the new partition-work setting is FE-only and needs no new wire field; existing split-source RPC semantics remain intact.
- Performance and memory: per-BE queues are bounded by split count, producer lanes and retained batches are bounded, and drains amortize locking over at most 64 descriptions. No concrete new performance bug was substantiated. Latency, throughput and fairness were not measured.
- Additional concerns: the final sweep rechecked every supplied changed path, candidate disposition and initial risk. No unresolved code-specific question or uncovered static-review scope remains.
No additional user focus was supplied. Validation was static inspection only: no builds, tests, benchmarks or runtime checks were run, and no repository source was modified. Convergence describes review coverage and candidate closure; it does not establish runtime performance or test success.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28905 ms |
…ests ### What problem does this PR solve? Issue Number: None Problem Summary: External scans now use batch mode by default. Batch EXPLAIN intentionally omits per-backend delete-file counts because only an initial subset of splits is planned. The Iceberg position-delete regression expected deleteFileNum from the default EXPLAIN and failed despite all result checks passing. Verify the approximate batch EXPLAIN separately, and request synchronous planning only in tests that assert complete delete-file detail. ### Release note None ### Check List (For Author) - Test: Manual test. Inspected the failed External Regression stacktrace and the FileScanNode/PluginDrivenScanNode EXPLAIN gate; git diff --check passed. External regression rerun is pending CI. - Behavior changed: No (test expectations only) - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
There was a problem hiding this comment.
Reviewed PR #69683 at head 0a415e5. No new actionable findings were verified across all 25 changed files and 76 diff hunks.
The existing P2 awaited-split terminal-state finding is fixed at this head: both awaited and bulk-drained splits recheck assignment failure/cancellation after capacity callbacks. The new parameterized test covers the one-split failure and cancellation cases. This issue is not reposted. No existing P0/P1 blocker remains to carry forward. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and evidence: the implementation separates the batch-mode trigger from Hive metadata batch size, bounds each BE queue by individual split count, and uses smaller producer batches. Unit and regression assertions cover the intended behavior; this review did not execute them.
- Scope and reuse: changes are focused on split scheduling, settings, the SPI extension, documentation and related tests. Blocking and resumable producers share the same queue implementation.
- Concurrency and locking: planner, SPLIT workers, FE fetch RPC threads and query cleanup were traced through the BE consumer. Queue state uses its per-BE monitor, assignment policy uses a separate lock, and producer scheduling flags use scheduleLock. Capacity callbacks run after queue/listener locks are released. No new lock inversion or external I/O inside those queue locks was found.
- Lifecycle: residual offsets retain each converted split once; completion removes listeners and closes each source serially. Stop publishes terminal state and signals waiters before resource cleanup. The shared finalizer waits for dispatch and all admitted lanes. No new static initialization dependency or ownership cycle was found.
- Configuration: split_partition_batch_size defaults to 16, rejects nonpositive values, is forwarded and exported to connector sessions generically, and is captured per query without restart. Effective producer size is clamped to queue capacity. The changed trigger, batch-size and queue-unit defaults are intentional and documented in the PR release note.
- Compatibility: connector API major 25.0, its pinned test and recorded plugin surface agree; the loader rejects mismatched plugin majors. ConnectorMetadata is unchanged. FE-BE fetch request/status/EOF semantics and storage formats are unchanged.
- Thrift/protobuf: no definitions or field IDs change; cross-branch field-ID comparison is not applicable.
- Parallel paths: both blocking and resumable enqueue paths and both bulk and awaited fetch paths were checked. Hive/MaxCompute partition batching and Hive/Iceberg streaming were traced; Paimon/Hudi and other non-opted-in connectors remain synchronous.
- Conditional guards: explicit batch-mode disable, empty/no-slot scans, sampling, deferred Variant checks, exact partition-metadata rules and Iceberg metadata-count/v3/system-table exclusions were checked. Hive partition claims remain disjoint and use positive operational batch/worker inputs.
- Test coverage: source inspection covers partial enqueue/resume, exact split identities, queue bounds, terminal races, cancellation, EOF, independent backend channels, Hive metadata batches and concurrent listing. Existing Hive regression cases exercise empty/pruned scans, aggregation, LIMIT, joins, unions, CTAS and statement hints. Runtime coverage remains unverified.
- Test results: no golden output is modified. Iceberg EXPLAIN checks distinguish approximate asynchronous split estimates from synchronous delete-file details. The Paimon synchronous hint is harmless. No new incorrect expected result was identified statically.
- Observability: generation profiles report the effective producer batch size and queue capacity; existing HMS batch metrics, split-source RPC error context and cleanup logs remain available. No additional observability defect was identified.
- Persistence and failover: no new persisted state or EditLog/transaction protocol changes are introduced; scheduling state is query-scoped.
- Data writes and atomicity: production changes affect scan planning and buffering, not commit/write atomicity. Cancellation and completion retain the existing cleanup boundaries.
- FE-BE variables: the new setting is consumed in FE planning and needs no new BE payload field; existing BE batch request limits and minimum split hints are preserved.
- Performance and memory: per-split queues eliminate the batch-size multiplier in the queue bound; each producer retains at most one assigned batch. Bulk draining amortizes synchronization, Hive metadata requests stay bounded, and remote BE scanner concurrency is not capped by a small approximate partition count. Benchmark claims were not independently validated.
- Core correctness and other issues: snapshot/rewrite pinning precedes asynchronous source creation, errors reach non-OK RPC status, and no new visibility, deletion or split-loss/duplication defect was verified. No internal visible-version/delete-bitmap logic or BE allocation accounting changes are involved.
Review converged in 1 round: all three assigned reviewers returned NO_NEW_VALUABLE_FINDINGS; every candidate and risk focus has an evidence-backed disposition, and the final coverage sweep found no remaining code-level gap. Validation was static only: no builds, tests, benchmarks or runtime stress checks were run, and repository source was not modified.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28683 ms |
Contributor
TPC-DS: Total hot run time: 152184 ms |
Contributor
ClickBench: Total hot run time: 24.3 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Problem Summary: With partition.cache.ttl-second=0, Hive partition-name reads bypass cache, but the derived partition view still used its default 24-hour cache. Batch split planning slices that stale view, so an externally added partition was omitted from a full scan. The external regression returned only the old partition after a new partition was inserted. Let the derived view inherit the effective partition-name TTL when no view TTL is specified. Tests cover both the legacy and namespaced zero-TTL settings. ### Release note Hive partition views now respect the partition-name cache TTL when a separate partition-view TTL is not set. ### Check List (For Author) - Test: Unit Test. HiveConnectorPartitionViewCacheTest: 5 tests passed with JDK 17 using existing worktree classes and a JUnit launcher; the repository run-fe-ut.sh exited in env.sh before Maven, and direct module Maven was blocked by unresolved snapshot reactor artifacts. External regression rerun pending CI. - Behavior changed: Yes (disabled Hive partition-name caching also disables derived view caching unless explicitly configured otherwise) - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
One P2 availability regression remains: early dispatch of small Hive partition scans can turn a slow, still-running partition listing into a fatal split-fetch timeout. The inline finding traces a two-partition case end to end.
Static review covered all 27 changed paths and the relevant planner, connector, producer, RPC and BE consumers. No builds, tests or benchmarks were run.
Applicable critical checkpoints:
- Goal: split-count queue bounds, smaller producer batches and independent Hive partition batches are implemented; the default-on path still has the timeout interaction reported inline.
- Scope and reuse: changes are focused on scan scheduling, its cache prerequisite, SPI/config wiring and tests; the shared producer and cache helpers are reused.
- Concurrency: producer tasks serialize residual offsets; queue contents/waiters use a per-backend monitor, policy assignment uses its own lock, and listener registration retries capacity. IO and resume callbacks stay outside queue locks; no new lock cycle was established.
- Lifecycle: traced initialization, first-sample dispatch, producer completion, EOF, error and cancellation through FE RPC and BE consumption. Early dispatch versus the fetch deadline is the remaining defect; cleanup removes listeners and closes sources.
- Configuration: positive-size checking, generic session extraction, statement forwarding and effective batch-size clamping are wired. New planning sessions observe overrides without restart.
- Compatibility: connector API major 25 and the plugin surface baseline agree; the metadata-method surface is unchanged. FE/BE fetch messages and stored formats are unchanged.
- Thrift/protobuf: no definition changes, so field-ID compatibility checks are not applicable.
- Parallel paths: checked yielding and blocking producers, partition and whole-scan sources, Hive/MaxCompute partition batching, Iceberg streaming gates, and non-opted-in connectors. Sampling and deferred Variant checks retain synchronous planning.
- Conditions: terminal checks, absent-key TTL precedence, positive size bounds and count/system-table gates have concrete purposes. Explicit partition-view TTL wins and refreshed partition records reach the engine conversion cache.
- Test coverage: inspected partial offers, multi-backend identity, queue bounds, callback races, awaited failure/stop, EOF, Hive lane concurrency, TTL bypass, config validation and regression cases. A slow partition with consumption beginning before listing completion is missing.
- Test results: changed assertions and EXPLAIN expectations match split-count queues and asynchronous profiles. No golden result files changed; inspected tests are not claimed as executed.
- Observability: existing split counters, partition profiles and FE RPC errors remain available. Benchmark claims were not independently validated.
- Persistence/failover: no EditLog, stored metadata format or transaction protocol changes.
- Data writes/crashes: production write atomicity, MoW/delete bitmap and visible-version logic are untouched; scan stop/failure paths were checked for retained producer work.
- FE/BE variables: the new partition-size knob is FE planning state and is forwarded between FEs; no new BE wire field is required.
- Performance/memory: queues are bounded by split count, producers retain one residual assigned batch, and bulk drain limits each lock hold to 64 entries. No separate substantiated performance defect was found.
- Other issues and user focus: no additional user focus was supplied. Existing thread 4387198347 is fixed by the awaited-split terminal recheck and its parameterized failure/cancellation test; it is not reposted. No existing P0/P1 blocker remains to carry forward.
Static-only limits: runtime timing and performance were not validated.
Review converged after 2 round(s).
| + "mode.", | ||
| needForward = true) | ||
| public int numPartitionsInBatchMode = 1024; | ||
| public int numPartitionsInBatchMode = 1; |
Contributor
There was a problem hiding this comment.
[P2] Reconcile early dispatch with the one-second split-fetch deadline
With the new defaults, two selected Hive partitions become two one-partition producers. If producer A returns its sole range while producer B is still doing HMS/directory IO, SplitAssignment.init() releases the planner on A's sample. FileQueryScanNode creates a SplitSource for every BE, so a BE without a range waits only the default fetch_splits_max_wait_time_ms=1000; while B is still listing, SplitSource.getNextBatch() throws and the BE treats the resulting RPC error as query failure. Even one BE can hit this after consuming A. Previously this tiny scan either planned synchronously or used one streaming producer whose default batch of 128 collected both ranges before publishing the sample. Please reconcile the normal fetch wait with unfinished listing before enabling these defaults, and cover two partitions with one listing blocked beyond the fetch deadline after the other publishes its range.
Contributor
Author
There was a problem hiding this comment.
Fixed in 47241e4. When the per-RPC wait expires while split production is still running, FE now returns INCOMPLETE instead of a fatal timeout or an empty EOF batch. BE retries after checking query cancellation and releases the FE client between attempts. Completion still returns EOF, and producer failures remain errors. SplitSourceTest now covers a second partition that produces after the wait window, a backend with no initial assignment, EOF, and failure after a pending fetch (37/37 passed). The FE Maven reactor and Checkstyle passed; the BE build is left to buildall CI.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69683 Problem Summary: With the new batch-mode defaults, a Hive scan can dispatch after its first partition produces a split. If another partition is still listing files, a BE may exhaust its assigned ranges and reach the one-second split-fetch wait limit. FE previously returned a fatal timeout even though split production was still running. FE now returns a retryable INCOMPLETE status only for this temporary gap; BE retries after checking query cancellation and releases its FE RPC client between attempts. Completed scans still return EOF, while producer errors and cancellation retain their failure paths. The wait setting remains a per-RPC bound and now rejects nonpositive values to avoid a tight retry loop. ### Release note External batch scans no longer fail solely because another partition takes longer than one split-fetch wait interval to list files. ### Check List (For Author) - Test: Unit Test: SplitSourceTest 37/37 passed with the repository test script. FE Maven build and Checkstyle passed through build.sh --fe; the outer script returned nonzero during output packaging. BE clang-format 16 check and git diff --check passed. BE compilation was not run locally. - Behavior changed: Yes. A temporary split gap produces a retryable response instead of a fatal timeout. - Does this need documentation: No. The session variable description was updated in code.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of all 29 changed files at 47241e4 found two new P2 regressions: an ungated pending response breaks old BE callers during rolling upgrades, and the BE retry loop ignores local scanner termination. Both findings are inline. No existing P0/P1 finding remains to carry forward.
Existing review context: thread 4387198347 is fixed by the awaited-split error/stop recheck. Thread 4396124903 is fixed for matching upgraded FE/BE versions by the pending-response/retry path; the distinct old-caller compatibility gap is reported inline. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: individual-split queue bounds, independent Hive partition batches and earlier asynchronous dispatch are implemented. Inspected tests target these controls and late production; mixed-version and local-stop cases are missing.
- Scope and clarity: production changes and adjacent partition-view TTL repair are focused on external scan scheduling.
- Concurrency and locking: FE producers, RPC consumers and cleanup share explicit queue/readiness/listener/resource synchronization; offsets preserve exact-once partial offers and callbacks run outside queue locks. No new lock inversion found. The prolonged BE mutex/worker hold after local stop is the second finding.
- Lifecycle: producer EOF/error/cancellation and query-finish source cleanup are wired; local scan termination does not reach the retry loop. No static initialization change.
- Configuration: new partition size and fetch interval have positive checkers; session capture/forwarding is reflective, so SET/SET_VAR reaches planning without restart. Queue capacity now counts splits; the effective producer batch respects that capacity.
- Compatibility: connector API and recorded plugin surface consistently advance to 25.0. The existing fetch RPC lacks pending-response capability negotiation, causing the first finding. No storage-format or function-symbol change.
- Thrift/protobuf IDs: no definitions changed.
- Parallel paths: resumable and blocking producers, bulk and awaited drains, Hive and MaxCompute partition batching, Hive/Iceberg streaming, synchronous fallback, and both BE file scanners were traced.
- Special conditions: offset preconditions, pending-versus-EOF, terminal checks, TTL precedence, sampling/Variant/count/snapshot fences have concrete purposes and preserve their intended paths.
- Test coverage: inspected queue, producer, concurrency, scan-node, connector, SPI and session tests plus changed regression suites. FE pending tests do not exercise the BE retry or an older caller. No tests or builds were run under this static-only contract.
- Expected results: no generated result files changed; modified EXPLAIN checks correctly request synchronous planning for full delete-file counts and approximate split reporting for streaming.
- Observability: effective batch size, queue capacity and GetSplitTime remain visible; existing errors preserve diagnostics except the old caller's unknown-error response described inline.
- Transactions and persistence: no EditLog, replay, commit or visible-version logic changed; existing snapshot/read fences remain.
- Data writes and crashes: production changes concern transient read-side scheduling; no persistent write atomicity change or additional data-loss path found.
- FE/BE passing: the new partition setting is FE-only and reaches connectors; source IDs and request limits remain unchanged. Pending-response support is not negotiated.
- Performance and memory: queues remain bounded per backend, lanes retain bounded in-flight batches, and bulk operations amortize locks. Clients return to cache between RPC attempts. Stopped scanners can nevertheless retain workers and fragment state as reported.
- Additional sweep: all changed paths, existing threads, candidates and three initial risk areas were rechecked; no other verified issue or unresolved source-level question remains.
The review converged after two rounds: all three reviewers returned NO_NEW_VALUABLE_FINDINGS in the final round, coverage is complete, and all candidates and risk items have evidence-backed dispositions. Validation is limited to source and test inspection; runtime, build, benchmark and test execution were not performed.
Review converged after 2 round(s).
| return result; | ||
| } catch (SplitSource.SplitPendingException e) { | ||
| // The backend will retry; an empty OK batch would incorrectly mean EOF. | ||
| result.status = new TStatus(TStatusCode.INCOMPLETE); |
Contributor
There was a problem hiding this comment.
[P2] Gate the pending response on backend support
A pre-change BE treats every non-OK split-fetch status as fatal, so during a rolling upgrade this unconditional INCOMPLETE makes the query fail with unknown error. No request capability or BE-version check protects it. With the new defaults, two Hive partitions can trigger this: A publishes its range while B lists past the one-second window. Before this PR, that small scan was synchronous, or one streaming producer collected both ranges before dispatch. The existing deadline thread is fixed for upgraded BEs; older callers still need a compatible path. Please negotiate support or gate asynchronous dispatch for old BEs, and cover upgraded FE + pre-change BE.
Contributor
Author
There was a problem hiding this comment.
This PR targets master only, with no release-branch backport. Doris upgrades BE before FE, so an FE containing this change is deployed only after the BE that understands the INCOMPLETE retry response. Upgrading FE first against a pre-change BE is outside the supported order for this master-only change; there is no intermediate master release to support. I am keeping this PR focused and am not adding capability negotiation for that unsupported pairing.
| ? "unknown error" | ||
| : result.status.error_msgs[0]); | ||
| do { | ||
| RETURN_IF_CANCELLED(_state); |
Contributor
There was a problem hiding this comment.
[P2] Let local scanner stop interrupt pending retries
RETURN_IF_CANCELLED only observes RuntimeState/query cancellation. A local LIMIT calls ScannerContext::stop_scanners, which sets scanner _should_stop without cancelling the enclosing query; FileScanner and FileScannerV2 check that flag only after this call returns. With parallel scanners (for example, a nested LIMIT above the serial-read threshold), A can satisfy the limit while B waits here for a slow partition and another query branch continues. B now retries across every wait window while holding _range_lock, a scan worker and the fragment context until listing or the entire query ends. Previously this call returned after one bounded fetch. Please observe local scan termination between retries or return control to the scanner, and cover this with RuntimeState still healthy.
Contributor
Author
There was a problem hiding this comment.
Fixed in 1f23f5b. Scanner::try_stop now publishes an atomic local stop signal, and FileScanner/FileScannerV2 pass it to the shared SplitSourceConnector. RemoteSplitSourceConnector checks it before each pending-response retry and returns no next range when a local LIMIT stops that scanner, even if RuntimeState and other query branches remain active. The new BE test covers a locally stopped scanner with an uncancelled RuntimeState. Header hygiene, clang-format 16, and diff checks pass. Local BE compilation and the test were blocked at CMake compiler-link detection by this Mac’s unaccepted Xcode license; buildall CI has been requested.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69683 Problem Summary: A LIMIT can stop a local scanner while other branches of the same query continue, leaving RuntimeState uncancelled. After a split-fetch RPC returns INCOMPLETE, RemoteSplitSourceConnector previously retried until more splits arrived or the query ended, holding a scan worker despite the scanner already being stopped. Pass the scanner's stop signal through SplitSourceConnector, make the signal atomic for cross-thread publication, and exit the retry loop with no next range when the scanner stops. FileScanner and FileScannerV2 use the same contract. ### Release note LIMIT queries stop waiting for additional external splits when their local scanner is finished. ### Check List (For Author) - Test: Added a BE unit test for local scanner stop while RuntimeState remains healthy. The test was not run locally: build.sh --be stopped during CMake's compiler link probe because this Mac has not accepted the Xcode license. Header hygiene, clang-format 16 validation, and git diff --check passed. Remote buildall is requested below. - Behavior changed: Yes. A stopped scanner no longer continues retrying pending split fetches. - Does this need documentation: No.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 37 changed files at head 1c7a1ac and traced connector production through FE queues/RPCs to both BE scanners. One new P2 finding: the longer first-split planning wait does not observe cancellation of an already-running producer. No still-applicable existing P0/P1 finding remains.
Critical checkpoint conclusions:
- Goal and evidence: individual-split queue capacity, smaller producer batches and connector-sized partition batches implement the stated early-delivery goal. Added unit/regression coverage was inspected; the cancellation gap in the inline finding remains.
- Scope and abstraction: changes are focused on split scheduling, its configuration, Hive partition-view TTL inheritance and associated tests/docs. Connector sizing remains behind the SPI.
- Concurrency: planner, fair scheduler producers, FE RPC consumers and BE scan workers were traced. Queue monitors protect deque state; backend assignment and capacity listeners use separate locks. Callbacks/connector IO run outside queue monitors; no new lock inversion or partial-offer loss/duplication was established.
- Lifecycle and memory: per-backend deque counts and bounded producer residuals limit retained split counts, rather than bytes. BE storage remains allocator-aware under its scan task context. Client connections release per retry; scanner stop is atomic. Pre-first-split query cancellation remains the reported lifecycle defect. No new cross-translation-unit static initialization dependency exists.
- Configuration: split_partition_batch_size is positive-checked and forwarded in connector sessions; effective producer size is min(batch size, queue capacity). Settings apply to new query planning without process restart. Queue capacity now measures splits, as documented.
- Compatibility: connector API major and recorded plugin surface both advance to 25. No new wire field is required. The existing old-BE/new-FE concern is already covered in a prior thread; the official upgrade flow requires BE before FE, and the new BE accepts the previous FE responses.
- Thrift/protobuf IDs: no gensrc changes; field-ID/enum migration checks are not applicable.
- Parallel paths: FileScanner and FileScannerV2, local/remote connectors, exception and WAL stubs, cooperative/blocking producers, Hive/MaxCompute partition batching and Iceberg streaming were inspected. Synchronous opt-outs remain intact; remote scanners are not capped by the partition-count estimate.
- Conditions and errors: pending is distinct from EOF, producer failure and cancellation; drain and awaited-split branches recheck terminal state. New stop/pending branches explain concrete scenarios. No additional silent-error path was established.
- Test coverage: inspected tests cover partial enqueue identity, queue-registration races, completion/error/stop, per-backend requests, delayed initial production/deadlines, TTL aliases and Hive end-to-end scan forms. A running first producer cancelled through StmtExecutor is not covered. The added BE test covers local stop before fetch, not an actual pending-RPC sequence.
- Test results: no .out result files changed. Changed expected values and EXPLAIN assertions were inspected; detailed delete-file counts intentionally request synchronous planning. No builds, tests, benchmarks or runtime validation were run for this review.
- Observability: existing GetSplitTime and generation profiles expose waits/batches; profile output uses the effective producer size. Errors remain propagated with existing logs; no additional mandatory metric was identified.
- Persistence/failover: no EditLog, persisted metadata schema or commit protocol changes; transactional persistence checkpoints are not applicable.
- Writes/atomicity: this change schedules reads and adjusts a derived metadata cache. No data-write or crash-recovery transaction path changed; read snapshot/lease preparation remains in the existing path.
- FE/BE variable propagation: the new setting is consumed through the connector session SPI, not a new serialized FE/BE variable. Existing scanner call sites pass the local atomic stop signal.
- Performance: bulk queue operations amortize locking, bounded offsets avoid reconversion/reassignment, and partial offers use available capacity. No new unbounded growth or verified scanner-concurrency regression was found. The PR's benchmark claims were not independently reproduced.
- Additional checks and user focus: no additional user focus was supplied. Cache alias precedence and native-format dispatch were traced; no further verified issue or unresolved code-coverage question remains after the final recheck.
Prior threads were independently revalidated: WAL override, local scanner-stop retries, pending fetch handling, awaited terminal-state recheck and normal slow first-split production are repaired at this head. The new finding concerns cancellation during the extended planning wait and is distinct from those earlier issues. Static-only validation is the review's remaining limitation.
Review converged after 2 round(s).
| splitAssignment.init(); | ||
| // The first split can require a remote read session (for example, MaxCompute). Keep | ||
| // the planning wait bounded by the query's effective timeout instead of a fixed 30 seconds. | ||
| splitAssignment.init(ConnectContext.get().getExecTimeoutS() * 1000L); |
Contributor
There was a problem hiding this comment.
[P2] Stop the first-split planning wait when the query is cancelled
When a producer is already inside a slow Hive listing or MaxCompute read-session call, KILL QUERY closes its task group but leaves the running SplitProducer untouched: FairTaskExecutor.Group.close() only cancels running Future tasks, while this producer is a plain Runnable. There is no coordinator yet, and this::stop is registered only for query finish, so needMoreSplit() stays true and init() keeps the planning thread waiting after cancellation. This extends the former 30-second upper bound to the full query timeout (900 seconds by default). Register scan/assignment stop with the executor's cancellation callback, or check cancellation during this wait, and cover cancellation while the first producer is running.
Contributor
Author
There was a problem hiding this comment.
Fixed in 1ce495e. FileQueryScanNode now registers the current SplitAssignment with StmtExecutor.registerQueryCancellationCallback before starting the first producer. Cancellation stops the assignment, wakes init immediately, and closes producer resources even when the running producer is a plain Runnable in remote IO. After init wakes, the planner throws the executor's retained cancellation reason rather than treating the stopped assignment as an empty scan. A cancellation that precedes registration is handled by the callback handoff and init skips starting more work. SplitAssignmentTest covers stopping after the producer starts but before its first split (23/23 passed); FE Checkstyle passed.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69683 Problem Summary: A query cancelled before its first external split had no coordinator yet. The existing cancellation callback closed the connector task group, but a running split producer is a plain Runnable and SplitAssignment.init could continue waiting for the full query timeout after cancellation. Register the current split assignment with the statement executor's planning-stage cancellation callback. Stopping the assignment wakes the first-split wait and closes producer resources; the planner then propagates the recorded cancellation reason. A cancellation that predates registration stops the assignment immediately, so init does not start more split work. ### Release note Cancelling an external query while its first split is still being prepared stops the planning wait promptly. ### Check List (For Author) - Test: run-fe-ut.sh ran SplitAssignmentTest (23 passed), including a stop during the first-split wait. FE Checkstyle and git diff --check passed. The local test script used Homebrew gmake for generated sources and skipped the redundant Maven gensrc goal because the host Xcode license blocks system make. - Behavior changed: Yes. Query cancellation interrupts the first-split planning wait. - Does this need documentation: No.
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 37 changed files at 1ce495e44b5d2fbc18fb0d7a320215e099a59cda. No new actionable findings and no still-applicable existing P0/P1 findings were verified. The prior WAL override P1 (4399108229) is fixed; the awaited-split error, pending-fetch, local-stop, first-split deadline and planning-cancellation threads are also addressed at this head. No duplicate comments are posted.
The review converged in 1 round: both complete reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS. The main agent independently checked their evidence, all initial risk items and the final changed-file set. No unresolved static code question remains. No additional user focus was supplied.
Inspected flow: planner → bounded producer lanes → per-backend split queues → FE fetchSplitBatch → shared BE split connector → FileScanner/FileScannerV2, with query cancellation and local LIMIT traced separately.
Critical checkpoint conclusions:
- Goal and proof: the implementation enables smaller asynchronous scans, separates Hive partition request size from the trigger, bounds queues by splits, and treats temporary production gaps separately from EOF/errors. Focused unit and regression cases support those behaviors by inspection; they were not executed here.
- Scope and reuse: changes remain focused on scheduling, buffering, defaults and their tests. Partition sizing stays in the connector SPI; TTL inheritance reuses CacheSpec mapping rather than adding connector branches to generic planning.
- Concurrency and locks: offsets retain partial-batch ownership without reconversion/reassignment. Queue predicate/wait monitors, listener-registration retry and wakeupRequested handoff cover the reviewed races. Queue and registry locks contain bookkeeping; listing, callbacks and RPCs run outside those FE queue monitors. No new lock cycle was found.
- Lifecycle: assignment cancellation is registered before first production, wakes planning before resource cleanup and preserves the executor cancellation reason. Producer completion owns source closure; running external IO finishes cooperatively. BE request/result/client lifetimes remain within the synchronous call. No new static-initialization dependency or reference cycle was found.
- Configuration: the new positive split_partition_batch_size defaults to 16, is forwarded and captured for subsequent statements without restart. Effective producer batches are capped by queue capacity. Fetch wait is validated positive and describes a per-RPC window.
- Compatibility: the SPI surface and major version 25.0 move together; all in-tree BE virtual overrides/callers match. The existing INCOMPLETE compatibility thread (4396311548) remains a deployment constraint: new FE with old BE is not negotiated, and this master-only PR relies on the author's stated BE-before-FE upgrade order. That known issue is not reposted.
- Thrift/protobuf: no definitions or field/enum IDs change; no persisted format change is introduced.
- Parallel paths: both scanner versions, local/remote sources, both producer APIs, synchronous fallback, sampling/count gates and connector opt-ins were checked. Hive batches independently; MaxCompute retains the SPI's 1024-partition request default; Iceberg keeps count/v3 exclusions; Paimon remains synchronous. Hybrid scanner selection exclusions were also traced.
- Conditional checks: the new stop/pending branches explain the concrete failure scenarios. Positive sizes and nonempty partition gating establish the batching arithmetic's domain; no speculative fallback masking an invariant violation was identified.
- Tests: inspected cases cover partial offers, multi-backend identities, capacity handoff, terminal races, delayed initial production, cancellation, pending/EOF/failure, TTL aliases and independent partition batching. The code remains modular for these cases. Runtime integration limits are stated below.
- Expected results: no .out fixtures change. Regression edits distinguish approximate streaming EXPLAIN from synchronous complete delete-file counters; the inspected assertions match those paths.
- Observability: effective batch size, queue capacity, generated counts, fetch timing and existing errors/profiles remain available. No new critical diagnostic loss was identified.
- Persistence and failover: no EditLog, metadata replay, transaction commit or persistent-state path changes; these checkpoints are not applicable to the modified logic.
- Writes and core invariants: no data-write, visible-version or delete-bitmap protocol changes. Reviewed queue ownership preserves split identity, bounded buffering and fail-loud producer error propagation; no new crash/cleanup deadlock was verified.
- FE/BE value passing: the new sizing knob stays in FE session/connector properties. Pending uses the existing status enum; scanner_stop is an in-process atomic pointer. No additional wire field is needed.
- Performance and memory: individual split limits, bounded producer lookahead, retained partial offsets and bulk drain avoid unbounded buffering or repeated conversion. BE pending polls release the client between attempts and check query/local stop. No benchmark result is independently claimed.
- Other risks: Hive TTL target-key precedence, classloader pinning, source subset ownership and early-dispatch scanner selection were rechecked without another substantiated issue.
Validation limits: this was a static-only review. No builds, tests, benchmarks or runtime fault injection were run. Direct BE pending-to-success and mid-RPC local-stop coverage is absent from the new test, which checks a pre-stopped scanner; live connector/RPC integration and full executor-to-scan cancellation were not demonstrated here. These limits do not leave an unresolved static finding or an unreviewed changed file.
Review converged after 1 round(s).
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28993 ms |
Contributor
TPC-DS: Total hot run time: 151013 ms |
Contributor
ClickBench: Total hot run time: 24.34 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
CalvinKirs
approved these changes
Oct 29, 2026
morrySnow
approved these changes
Oct 29, 2026
5 of 16 tasks
OIiveirra
added a commit
to OIiveirra/doris
that referenced
this pull request
Oct 29, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#69032, apache#69683, apache#69799 Problem Summary: Upstream partition batching and the spatial SPI extension both used connector API 25.0. Merge official master while retaining both interface surfaces, advance the spatial extension to 26.0, and update its surface-test pin. Preserve both spatial constant-folding protection and upstream HLL exclusions. Apply the two approved HLL constant-result getter calls equivalent to apache#69799; do not change HLL algorithms or its tests. Spatial functionality and ordinary-key spatial-value support remain unchanged. ### Release note None ### Check List (For Author) - Test: Standard ASAN BE+FE build and Checkstyle; selected FE UT 219/219 (including SPI 153/153); GIS, Iceberg and VARBINARY native regressions; API26 resource and ten connector manifests; format16, hygiene and changed-line clang-tidy. BE UT compilation is blocked before execution by unchanged upstream HLL test references to the removed ColumnPtrWrapper; no passing BE UT is claimed. - Behavior changed: No. Preserve both parent features and distinguish the spatial SPI major version from upstream partition batching. - Does this need documentation: No. Internal merge/API reconciliation; public spatial contracts in apache/doris-website#4239 remain unchanged.
morningman
added a commit
that referenced
this pull request
Nov 2, 2026
…hema (#69488) ### What problem does this PR solve? Issue Number: None Related PR: #66538, #57531, #65851 Problem Summary: **In short.** On an Iceberg table, `DEFAULT(column)` anywhere except `UPDATE ... SET` and `MERGE ... VALUES` reads the default from the read schema, which carries no Iceberg write-default. This PR resolves it against the write schema that the statement pinned for its target, as branch-4.1 and branch-4.2 do. **Background.** An Iceberg `write-default` applies to rows written after it is set. So it lives in the writer schema that a write statement pins (`IcebergWriteSchemaContext`), not in the columns of the read schema. #65851 resolves `DEFAULT(column)` against that pinned schema in `UPDATE ... SET`, `MERGE ... UPDATE SET` and `MERGE ... INSERT VALUES` (`ConnectorWriteSchemaUtils`). Every other `DEFAULT(column)` either fails at binding or reaches `RewriteDefaultExpression` (#57531), which reads the default from the bound column. branch-4.1 #66538, also in branch-4.2, resolves these against the pinned write target too. It rejects `DEFAULT(column)` on an Iceberg column in a statement that does not write the table. **The problem, and what it cost.** Reproduced on a local cluster with a `file://` format-version 3 table: `CREATE TABLE t (id INT, c INT)`, `ALTER TABLE t ADD COLUMN d INT DEFAULT 42`, one row `(0, 0)`, so `d = 42`. | Statement | Master today | After this PR (same as branch-4.2) | |---|---|---| | `INSERT INTO t (id, c, d) VALUES (1, 1, DEFAULT(d))` | `Unknown column 'd' in 'table list'` | writes 42 | | `INSERT INTO t (id, c, d) SELECT 2, 2, DEFAULT(d) FROM t WHERE id = 0` | writes NULL | writes 42 | | `INSERT INTO t (id, c, d) SELECT 3, 3, DEFAULT(d)` | `Unknown column 'd'` | writes 42 | | `UPDATE t SET c = 100 WHERE d = DEFAULT(d)` | updates nothing: `d = NULL` makes the plan an empty set | updates every row, `d = 42` is pushed down to the scan | | `MERGE INTO t USING (SELECT 0 AS id) s ON t.id = s.id AND t.d = DEFAULT(t.d) WHEN MATCHED THEN UPDATE SET c = 300` | matches nothing | updates row 0 | | `SELECT DEFAULT(d) FROM t` | NULL | `DEFAULT(d) must reference a column of the table that this statement writes` | The `UPDATE` and `MERGE` cases fail silently: the statement succeeds and changes nothing. **How this PR fixes it.** - fe-core cannot tell from a column whether its connector keeps defaults in the read schema. Hive does: an HMS default shows up in both schemas. Iceberg does not. The new connector capability `SUPPORTS_WRITE_ONLY_DEFAULTS` says which kind a table is. The Iceberg connector declares it. The Hive connector passes it on to iceberg-on-HMS tables, like the other per-table capabilities, but only through the two-argument `getTableSchema`. Since #69299, a statement that pins the table at its latest snapshot reads the schema from the snapshot overload, which reflects none of them, so on an iceberg-on-HMS table such a statement keeps master's results (for example, `UPDATE t SET c = 100 WHERE d = DEFAULT(d)` still changes no row). Reflecting the sibling's capabilities there changes planning for all of them and is left to a follow-up PR. - For a target that declares it, `InsertUtils.normalizePlan` resolves `DEFAULT(column)` in `INSERT ... VALUES` and in the select lists of `INSERT ... SELECT`, including the replacements of `SELECT * REPLACE(...)`, against the pinned write schema. It does this by name, before the source relations are bound, so the name always refers to a target column, as in branch-4.2. In MySQL, `DEFAULT(column)` in a `SELECT` refers to a source column instead. A derived table in `FROM` is a query of its own and is left alone: `DEFAULT(column)` in it still names a column of its own `FROM` clause. - `RewriteDefaultExpression` takes the default for a `DEFAULT(column)` bound to such a table from the pinned write column with the same field id. That covers the `WHERE` of `UPDATE` and the `ON` of `MERGE`. A MERGE resolves it whatever its WHEN branches, also when they all delete, as branch-4.2 does. If the statement pinned no write schema for that table, it fails. A `DELETE` pins the writer schema too, for its delete sink, but it writes no row values, so `RowLevelDmlCommand` marks its target and `DEFAULT(column)` on it fails with `DEFAULT(d) cannot be used in a DELETE: the defaults of this table apply only to the rows that a statement writes`, as in branch-4.2, whose DELETE installs no write schema context. Master reads NULL there, so `DELETE FROM t WHERE d = DEFAULT(d)` deletes nothing; resolved to the write-default it would delete every row with `d = 42`. - A resolved default is cast to the type of its writer column before the expression around it is bound, as `RewriteDefaultExpression` already does for a bound `DEFAULT(column)`. The parsed literal `-1` is a TINYINT, so without the cast `bit_count(DEFAULT(c))` on an INT column would count 8 bits instead of 32. The external `UPDATE`, `MERGE` and changelog paths use the same helper. `RewriteDefaultExpression` rejects a bound `DEFAULT(column)` whose writer column has another type than the read column the statement was bound with, which happens only when the table changed outside Doris after Doris cached it, and asks to refresh the table: the expressions around it were bound against the read type. - Hive, Paimon and internal tables do not declare the capability, so they behave as before. - The new enum constant changes the connector plugin API surface, so `connector.plugin.api.version` goes from 26.0 to 27.0, as for the other recent changes to that surface (#69299, #69588, #69298, #69683 and #69835 took 22.0 to 26.0). **Where `DEFAULT(column)` resolves on an Iceberg target (direct Iceberg catalogs).** | Statement | Position of `DEFAULT(column)` | Result | branch-4.2 | |---|---|---|---| | `INSERT` / `INSERT OVERWRITE` | `VALUES` rows | the target column's write-default, typed as the column | same | | | the INSERT's own select lists (with or without `FROM`, set operation branches, `SELECT * REPLACE`) | the target column's write-default, by name, before binding | same, except `* REPLACE`, which branch-4.2 binds to the source | | | `GROUP BY` / `ROLLUP` outputs, a derived table in `FROM`, `WITH` definitions, subqueries, `WHERE` / `ON` | bound like any `DEFAULT(column)` (last row) | same, except derived tables, which branch-4.2 resolves against the target | | | a `VALUES` branch of a set operation | fails with an unknown column, as on master | same | | | a qualifier other than the target (`DEFAULT(src.c)`, `DEFAULT(ctl.db.t.c)`) in the positions above | fails: `column in assignment list is invalid` | same | | `UPDATE` | `SET` / `WHERE` | write-default | same | | `MERGE` | `ON` and `WHEN ... AND` conditions, whatever the WHEN branches, also when they all delete | write-default | same | | | `UPDATE SET` / `INSERT VALUES` | write-default | same | | `DELETE` (also `DELETE ... USING`) | anywhere | fails: `cannot be used in a DELETE` | fails: its DELETE has no write schema context | | any | a bound `DEFAULT(x.c)` | `x` is the written table: its write-default by field id; an OLAP or Hive table: that column's default; another Iceberg table: fails with `must reference a column of the table that this statement writes` | same | A bound `DEFAULT(column)` whose writer column has another type than the read column fails and asks to refresh the table. Where `DEFAULT(column)` is bound rather than resolved by name (the third row), it keeps its meaning from before this PR and names a column of its own `FROM` clause, as on master and branch-4.2: for example in `INSERT INTO t SELECT id, DEFAULT(d) FROM src GROUP BY id, d` with an OLAP or Hive `src`, it takes the default of `src.d`. **Results.** - Local cluster, with the table above: the table shows what each statement did before and after. `EXPLAIN UPDATE t SET c = 5 WHERE d = DEFAULT(d)` goes from an empty set to a scan with `d = 42` pushed down to Iceberg. - Unit tests: 6 new tests in `InsertUtilsTest`, 6 in the new `RewriteDefaultExpressionTest` and 1 in `RowLevelDmlCommandTest`. They cover VALUES, and INSERT ... SELECT both from a staging relation whose own column is also `name` and without FROM: each resolves to the pinned default, and an unknown column fails. `DEFAULT(column)` inside a derived table, qualified or not and also under a UNION branch, is left to binding. A `SELECT * REPLACE(DEFAULT(name) AS name)` replacement resolves like the rest of the select list and keeps the star's qualifier and `EXCEPT` list. The pinned write column wins over the read column. A statement that writes nothing fails. A table without the capability is unchanged. With master's `InsertUtils` and `RewriteDefaultExpression`, the 5 tests of the fix fail and the 2 tests of the unchanged path pass. The `SELECT * REPLACE` test fails with this PR's previous `InsertUtils`, where the replacement stays `DEFAULT(name)`. `IcebergConnectorTest`, `HiveConnectorMetadataSiblingDelegationTest` and `ConnectorPluginSurfaceTest` pass. So do the fe-core tests around the write schema: `ConnectorWriteSchemaUtilsTest`, `ExternalRowLevelUpdatePlanBuilderTest`, `ExternalRowLevelMergePlanBuilderTest`, `ChangelogRowLevelDmlTransformTest` and `PluginDrivenExternalTableTest`. Two of the tests pin the typing: `bit_count(DEFAULT(bits))` gets the default as an INT in VALUES and in SELECT, and fails without the cast; a BIGINT writer column over an INT read column is rejected, directly and under an IF bound to INT. `DEFAULT(column)` on the target of a DELETE fails even with a pinned write-default, and only a DELETE marks its target (an UPDATE does not); skipping the check, or marking UPDATE instead of DELETE, makes exactly these two tests fail. `PositionDeleteRowLevelDmlTransformTest`, `PhysicalPlanTranslatorIcebergRowLevelDmlTest`, `DeleteFromCommandTest` and `UpdateCommandTest` pass. - Regression: `test_iceberg_initial_defaults` now writes rows 20 to 22 with `DEFAULT(default_int)` in VALUES and in INSERT ... SELECT with and without FROM. It then updates them with `WHERE default_int = DEFAULT(default_int)` and `MERGE ... ON ... = DEFAULT(...)`, for Parquet and ORC, and checks the rows in the `.out`. A plain `SELECT DEFAULT(default_int)` must fail, and so must a `DELETE ... WHERE default_int = DEFAULT(default_int)` on rows 20 to 22, which hold the write-default. The suite passed locally before the DELETE block was added, and the whole suite, DELETE block included, has since passed in two External regression runs on CI. With master's FE it fails at the first of these statements: `Unknown column 'default_int' in 'table list'`.
924060929
added a commit
to 924060929/incubator-doris
that referenced
this pull request
Nov 2, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#69683 Problem Summary: Enabling batch planning for small external scans leaves Iceberg partition counts at zero and split counts approximate when SQL block rules run before execution. It also removes partition and delete-file details from EXPLAIN. Require the existing synchronous planning path for EXPLAIN and enabled applicable partition_num/tablet_num rules, with one shared eligibility gate for Hive and Iceberg. Keep ordinary unrestricted queries on bounded asynchronous split delivery. Rule applicability and the internal-query bypass remain owned by SqlBlockRuleMgr. ### Release note Preserve external-table partition and split SQL limits and complete EXPLAIN scan details when batch split planning is enabled. ### Check List (For Author) - Test: Unit Test - Reproduced the two eligibility regressions before the fix; all 196 related FE unit tests pass after the fix. - Full FE packaging and Checkstyle pass through build.sh --fe -j4 (UI build disabled). - External-service regression remains for remote CI; no BE code changed or BE validation rerun. - Behavior changed: Yes (use exact synchronous scan planning for EXPLAIN and applicable partition/split limits) - Does this need documentation: No
924060929
added a commit
to 924060929/incubator-doris
that referenced
this pull request
Nov 2, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#69683 Problem Summary: Hudi and MaxCompute bypassed common exact-planning eligibility, MaxCompute used the batch-mode threshold as its task size and queued all partition groups, and non-positive split-fetch waits caused immediate retries. A backend stopping at a local LIMIT could also leave its bounded FE queue blocking publication to the remaining backends. Apply the common eligibility gate, use a dedicated bounded MaxCompute continuation, validate the wait before starting producers, and release each stopped consumer from the scan operator owner through a capability-gated optional request. Preserve ordinary EXPLAIN batch plans, require full metadata for VERBOSE/admission checks, and retain native Hive listing root causes. ### Release note Fix hangs and excessive scheduling during lazy external-table scans, preserve complete scan-count admission and verbose plan metadata, and retain Hive missing-partition diagnostics. ### Check List (For Author) - Test: 205 related FE unit tests passed through run-fe-ut.sh; 60 related BE ASAN unit tests passed through run-be-ut.sh. Full FE package/Checkstyle and full BE ASAN package passed through build.sh. BE validation uses a local-only source-specific CMake warning adjustment and a focused native test target; neither is committed. External-service regression cases are updated and await CI. Changed-line clang-tidy is blocked by the existing unmatched NOLINTEND in be/src/core/types.h:578. - Unit Test / Regression test updates - Behavior changed: Yes; bounded split delivery now releases stopped backend consumers, validates wait intervals, and preserves exact planning where required. - Does this need documentation: No
924060929
added a commit
to 924060929/incubator-doris
that referenced
this pull request
Nov 2, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#69683 Problem Summary: Hive split generation now schedules several partitions per task, but recorded the whole task output as the split count of one partition. Extrapolating this across all pruned partitions inflated approximate split/file-size estimates: six partitions with ten splits each reported 360 instead of 60. Count actual output by partition values within each batch and atomically retain the largest per-partition count. Preserve the existing minimum and cover uneven/empty partitions, sampling, unpartitioned scans, and different scheduling batch sizes. Correct the legacy-FE closure comment and verify that its existing timeout error is propagated without retries or an unsupported close request. ### Release note Correct approximate Hive split counts and derived file-size estimates during batched external scans. ### Check List (For Author) - Test: 209 related FE unit tests passed through run-fe-ut.sh. The unchanged-production reproduction showed 360 instead of 60 and 116 instead of 100; all four new cases now pass. Full FE packaging and Checkstyle passed through build.sh --fe. 61 related BE ASAN tests passed through run-be-ut.sh, including the new old-FE timeout/no-retry case. BE test validation uses the existing local-only focused CMake target and source-specific compiler-warning adjustment, restored afterwards and excluded from this commit. - Unit Test - Behavior changed: Yes; approximate Hive estimates are independent of scheduling batch size and preserve the per-partition maximum. - Does this need documentation: No Changed-line clang-tidy remains blocked by the existing core/types.h:578 unmatched NOLINTEND and the pre-included gtest/internal/gtest-internal.h:56 float.h diagnostic.
924060929
added a commit
to 924060929/incubator-doris
that referenced
this pull request
Nov 2, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69683 Problem Summary: The split-consumer close concurrency test initializes the large Env and frontend RPC service mocks inside its 30-second method timeout. CI logs show RPC static initialization still running after that budget, before the producer thread is created. Move these class fixtures to BeforeAll, following existing bounded-concurrency tests, so cold initialization does not consume the queue-race budget. Keep the original 30-second timeout, five-second phase bounds, close-race assertions, RPC idempotence, and executor cleanup unchanged. ### Release note None ### Check List (For Author) - Test: Unit Test: controlled 1-second budget fails before the fixture move and passes afterward; 63 related FE tests pass with the original 30-second limit; full FE packaging and Checkstyle via build.sh --fe - Behavior changed: No - Does this need documentation: No
924060929
added a commit
to 924060929/incubator-doris
that referenced
this pull request
Nov 2, 2026
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69683, apache#69991 Problem Summary: Hive lazy partition listing applies TABLESAMPLE independently to every scheduling batch, resetting the ROWS byte target and percentage rounding. A sampled scan now uses the existing complete-metadata path to select files once across all pruned partitions, preserving its seeded table-wide selection. Remote split-source cleanup previously recorded completion before FE acknowledgement, so a temporary RPC failure could prevent subsequent cleanup and strand a bounded producer queue. Serialize close acknowledgements independently of consuming fetches and retry only the idempotent cleanup request through the standard Thrift reconnect path. Mark completion after acknowledgement; exhausted errors still cancel the query so other backends cannot wait indefinitely behind an unclosed queue. ### Release note Fix Hive TABLESAMPLE over-selection with partition batching and recover split-source cleanup from temporary RPC failures. ### Check List (For Author) - Test: Unit tests and packaging - 236 related FE tests passed; full FE packaging and Checkstyle passed. - 63 focused BE ASAN tests passed on the final lock/test-helper implementation. - Full BE ASAN packaging (including Java scanning extensions) passed. - clang-format 16 and git diff --check passed. Changed-line clang-tidy found no remaining new diagnostics; it remains blocked by the pre-existing core/types.h NOLINTEND and forced gtest float.h header diagnostics. - BE tests use a focused native target; the complete BE UT suite and external-service regressions were not run in this cycle. Local CMake build adaptations are excluded from this commit. - Behavior changed: Yes. Sample files are selected once over all pruned partitions; source closure retries temporary failures and records completion only after acknowledgement. Ordinary consuming fetches retain their existing transport-error behavior. - Does this need documentation: No
924060929
added a commit
to 924060929/incubator-doris
that referenced
this pull request
Nov 3, 2026
… explain ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69683, apache#69991 Problem Summary: The Iceberg Variant remote-gather fixture asserts inputSplitNum=2 to prove that its selected snapshot contributes both shredded and unshredded files. With external batching enabled by default, ordinary EXPLAIN intentionally reports a sampled approximate split count, and the external regression fails on approximate inputSplitNum=1 before executing the mixed-file result/profile checks. Select EXPLAIN VERBOSE for this single action through the existing regression DSL so the shared exact-planning guard enumerates both files. Preserve the exact split-count, merge-exchange and projected-path assertions, all subsequent data/profile checks, and default batching for executable queries. ### Release note None ### Check List (For Author) - Test: Existing FE unit tests and direct regression-call-chain review - The current-head external CI failure and exact revision were captured from build 1089454. - Both testVerboseExplainRequiresExactFilePlanning and testOrdinaryExplainKeepsTheRealBatchPlan passed using run-fe-ut.sh (2 tests, no failures/errors/skips). - Integrated FE Checkstyle, source hash verification and git diff --check passed. - The live external regression was not run locally: the configured Iceberg REST and MinIO endpoints are unavailable. New-head CI must validate the complete suite. - No FE/BE repackaging was needed for this regression-only DSL change. - Behavior changed: No product behavior change. Only the fixture EXPLAIN requests exact planning; executable queries retain their batching settings. - Does this need documentation: No
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Problem Summary: External scans currently enter Batch Mode only above 1024 partitions or files. Even in Batch Mode, a producer batch defaults to 128 splits and each backend queue is bounded by 32 batches. That couples the memory bound to batch size and delays small LIMIT scans while large groups are prepared.
This PR enables Batch Mode from one partition or file by default. It separates the Hive partition-processing batch size (
split_partition_batch_size, default 16) from the Batch Mode trigger. Producers still assign and offer splits in batches, but each backend queue stores individual splits and enforces a capacity of 1024 splits; a partly full queue accepts only the available slots and wakes the producer when the consumer drains it. The producer batch defaults to 8 splits. The consumer still obeys the BE request batch size, while preserving cancellation, first-fetch limits, backpressure, and error propagation. The new connector SPI method advances the plugin API version to 25 and updates its recorded surface.A slow Hive partition can outlast the one-second split-fetch RPC wait after another partition supplies the first split. FE now returns
INCOMPLETEwhile production is still running, and BE retries after checking cancellation. A completed assignment still returns EOF, and producer failures remain errors. If a local LIMIT stops a scanner while other query branches remain active, its BE retry loop observes that scanner stop and exits without waiting for more splits.In a matched, one-BE Release benchmark on the integrated test branch, refreshed-metadata LIMIT medians at
split_batch_size=8versus32were 75.8 vs 102.5 ms for Hive 10k partitions and 23.5 vs 30.6 ms for Iceberg 1000 manifests. Eight beat 32 in 11/12 paired blocks in each LIMIT scenario. Warm full scans had no stable winner among 8/16/32/64. These numbers select the producer batch size; they are not a fresh end-to-end benchmark of this rebased PR against current master.Release note
External table Batch Mode now starts at one partition or file by default. The default producer batch size is 8 splits, queue capacity is 1024 splits per backend, and
split_partition_batch_sizecontrols Hive partition-processing batches separately.Check List (For Author)
SplitSourceTestpassed 37/37 throughrun-fe-ut.sh. The Maven reactor and Checkstyle passed throughbuild.sh --fe; its outer output-packaging step returned nonzero. The changed BE sources passed clang-format 16, header-hygiene validation andgit diff --check.build.sh --bewas attempted but stopped during CMake compiler-link detection because this Mac has not accepted the Xcode license, so the new BE test has not run locally. CI build is requested below.