Repository navigation
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: N/A Problem Summary: Bucketed aggregation always allocates 256 hash tables, even for serial execution, and its bucket-routing bits overlap phmap probe-index bits as tables grow. Add a validated and forwarded bucket-count session variable, capture it in an optional plan field, choose one bucket for a single sink and source, and use high mixed-hash bits for routing. Reuse batch insertion for one bucket, avoid unused inline-count storage and skip single-sink merging. Add configuration, concurrency, plan-translation and SQL regression coverage. ### Release note Add bucketed_agg_num_buckets: 0 automatically chooses one bucket for a single sink and source and 256 otherwise; explicit values must be powers of two in [1, 256]. Missing Thrift fields retain 256 buckets. SQL aggregation semantics are unchanged. ### Check List (For Author) - Test: FE unit tests (35 passed); clang-format 16; FE Checkstyle; BE build hygiene; git diff --check; independent audit of 120 supplied golden sections. BE unit tests blocked by missing third-party libraries; regression execution and complete clang-tidy analysis remain pending. - Behavior changed: Yes (automatic bucket counts and per-session override; unchanged query results) - Does this need documentation: Yes (session-variable description included; website documentation PR pending)
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review of head e2d4c6228663149235f13faf991b8c1e679a62ae found one P2 issue: the new regression suite must disable both spill flags before requiring bucketed aggregation, because ordinary p0 fuzzy sessions can enable them. The inline comment contains the trigger and fix. No production-code defect or P0/P1 finding was verified, and there are no existing inline findings to carry forward.
Execution flow reviewed: session SET/forwarding → immutable plan value → optional Thrift field → shared initialization → independent sink tables → EOS publication → bucket merge/output → NULL consolidation → cleanup.
| Required checkpoint | Conclusion |
|---|---|
| Goal and evidence | Automatic/explicit bucket counts and serial shortcuts are consistent in the inspected flow. Unit and regression coverage is supplied; the regression prerequisite issue needs correction. Runtime success and performance gains are not established by this static review. |
| Scope | All 14 changed paths covered: BE operators/shared state and unit tests; FE configuration, translation/node and unit tests; Thrift; regression suite and output. Changes are focused on bucketed aggregation. |
| Concurrency | Checked task creation, pre-registered dependencies, call_once, per-sink ownership, EOS release/acquire publication, per-bucket CAS and final NULL claim. No new lock order or synchronization defect verified. |
| Lifecycle/static initialization | Tables and sink/source arenas remain query-owned through output and destruction. The single-sink shortcut skips an otherwise empty merge loop; LIMIT/close retains bucket release. No new global initialization or ownership cycle. |
| Dynamic configuration | Common SET validation rejects invalid values before assignment; forwarding and plan capture preserve the chosen value without restart or dependence on later session restoration. |
| Compatibility | Missing num_buckets keeps 256 on a new BE; an old BE ignores the optional count. No stored bucket layout or storage-format change. Existing bucketed-node version gating remains. |
| Thrift IDs | Read the touched definition at HEAD and on origin/branch-3.1, branch-4.0, branch-4.1, and branch-4.2. The struct is absent on those release snapshots; HEAD adds unused optional field 6. No collision or required-field issue found. |
| Parallel/special paths | Checked numeric, string, fixed and serialized composite keys; nullable/const keys; ordinary and inline COUNT states; asymmetric task counts; one/many buckets; empty input, multiple batches and LIMIT. Active loops use the selected count. |
| Conditions and errors | Range/power-of-two validation protects mask routing and fixed upper bounds. Automatic selection accounts for source fan-out. Invalid requests use existing error paths. |
| Test coverage | Added BE tests cover routing/count selection/concurrent initialization/invalid values; FE tests cover validation/forwarding/immutable capture; SQL matrix covers ten query kinds across two parallelisms and six counts, plus LIMIT and invalid SET. The inline P2 identifies why normal fuzzy CI can fail before exercising positive coverage. |
| Expected output | Independently recomputed all 32,768 input rows and checked every one of the 120 supplied output sections: no mismatch. Generation provenance and runtime regression success cannot be established statically. |
| Observability | EXPLAIN displays the requested count; NumBuckets, NumSinkInstances and PartitionTime expose effective setup and partition work. |
| Transactions/persistence | No transaction, EditLog, visible-version, delete-bitmap or rowset behavior changed. Session persistence uses existing annotated-variable infrastructure. |
| Data writes/failure cleanup | No new data-write protocol. Reviewed state destruction, cancellation/early close and NULL ownership; no new leak or atomicity defect verified. |
| FE/BE propagation | The sole production node constructor and Thrift creation site capture/send the value; sink initialization consumes and validates it. Point-query and constant-folding paths do not instantiate this grouped operator. |
| Performance/memory | The shortcuts avoid scatter, redundant merge visitation and unused COUNT places storage. High-byte routing receives a mixed hash, checked against the pinned phmap v1.3.8 hash implementation and mixer, plus the local ARM64 patch. Active-table reservation/accounting remains in place. No benchmark was run. |
| Other concerns | Fast-path, hash-width/NULL-routing and dependency/task-count risks have explicit evidence-based dispositions. No additional user focus was supplied. No other verified finding or uncovered changed path remains. |
Validation limits: static inspection and independent expected-result recomputation only; no builds, Doris tests, cluster execution or benchmarks were run under this review contract. These limits do not imply runtime validation.
Review converged after 2 rounds: all three reviewers returned NO_NEW_VALUABLE_FINDINGS on the unchanged final inline set. Every candidate and risk focus has an explicit disposition, and no changed path remains uncovered.
Review converged after 2 round(s).
| sql "set agg_phase=1" | ||
| sql "set bucketed_agg_min_input_rows=0" | ||
| sql "set bucketed_agg_max_group_keys=0" | ||
| sql "set bucketed_agg_high_card_threshold=1.0" |
There was a problem hiding this comment.
[P2] Disable spill before requiring the bucketed plan
The p0 FE configuration enables fuzzy sessions, and SessionVariable.initFuzzyModeVariables() randomly turns on enable_spill and enable_force_spill. AggregateUtils.isBucketedHashAggEnabled() returns false when either is enabled, so this suite's first contains "VBUCKETED AGGREGATE" fails for those normal CI sessions and the bucket-count matrix never runs. Set both flags to false in the setup, as the existing bucketed_hash_agg suite does.
|
run buildall |
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69425 Problem Summary: The subquery translation unit test constructs a translator context without a session, so reading the configured bucket count throws a null pointer exception. Reuse the planner's CascadesContext, following existing translation tests. The bucket-count regression suite also requires a bucketed plan while fuzzy sessions may enable spill, which disables that plan. Explicitly disable both spill flags before exercising the bucket-count matrix. ### Release note None ### Check List (For Author) - Test: Unit Test: 50 tests passed across AnalyzeWhereSubqueryTest, BucketedAggregateTranslatorTest and SessionVariableBucketedAggTest; no failures, errors or skips. Runtime regression remains for CI. - Behavior changed: No; test setup only. - Does this need documentation: No.
|
/review |
|
run buildall |
There was a problem hiding this comment.
Static review of PR #69425 at 2ea21cef7faab6191d85ea7ae2ca370f90ed33ef: no substantiated new findings. The full 15-file diff was reviewed across BE execution, FE planning/configuration, protocol compatibility and tests, with a separate review of hash routing and the single-bucket fast path.
Execution flow reviewed: session setting -> captured plan value -> optional Thrift field -> once-only shared initialization -> independent sink tables -> sink completion publication -> per-bucket source merge/output -> NULL consolidation -> shared-state cleanup.
| Required checkpoint | Conclusion |
|---|---|
| Goal and proof | Automatic/explicit counts reach allocation, routing, merging and output. Added FE/BE tests and a differential SQL matrix target this behavior; this invocation provides static evidence only. |
| Scope and clarity | Changes focus on bucketed aggregation and its setting/protocol/tests. The subquery test now supplies the planner context needed for session access. |
| Concurrency and locks | Pipeline tasks remain the concurrent actors. call_once publishes shared metadata/counts, EOS uses release/acquire, and per-bucket CAS protects merge/output. No new mutex or reverse lock order; the single-sink shortcut preserves ownership and completion gates. |
| Lifecycle and static initialization | Sink/source arenas and aggregate states remain query/shared-state owned. Cleanup visits allocated vectors and preserves counter-versus-state destruction. No new ownership cycle or cross-TU initializer. |
| Configuration and dynamic changes | Standard session/global validation accepts 0 or powers of two through 256, rejects invalid values before mutation, and forwards the setting. New plans observe changes without restart; existing plans retain their captured value. |
| Compatibility | Optional node field; a new BE defaults to 256 when it is absent, and an older BE ignores it while retaining its existing layout. No new function symbol, storage format or transmitted aggregate-state layout. |
| Thrift IDs across releases | Compared the touched definition with live-verified branch-3.1/4.0/4.1/4.2 and master snapshots. Releases lack this struct; master has fields 1,2,3,5. New optional field 6 has no collision. |
| Parallel and special paths | Active bounds cover sink initialization/reservation/profiling, source traversal/completion and NULL consolidation. Numeric/string/fixed/serialized and nullable variants were traced. The sole FE construction path captures the value; regular/spill/cache/unsupported paths retain their existing gates. |
| Conditions and comments | Range/power-of-two validation protects mask routing and fixed maximum state storage. Automatic selection considers sink/source multiplicity. Fast-path branches explain why scattering/merging/places storage can be skipped. |
| Test coverage | BE tests cover routing bit separation, configured counts, concurrent initialization and invalid input. FE tests cover SET/forwarding/capture. SQL covers multiple batches, NULLs, string/numeric/composite groups, inline COUNT, distinct groups, empty input, LIMIT, invalid SET and count/parallelism combinations. Runtime execution remains unverified here. |
| Changed expected results | Independently recomputed the 32,768 input rows and exactly matched every row in all 120 golden sections, including NULL-first lexical ordering. No mismatch. Static reading cannot establish how the file was generated. |
| Observability | Configured count appears in EXPLAIN; actual count, sink count and partition time appear in profiles. Existing merge/memory counters remain in use. No new noisy logging path. |
| Transactions and persistence | No transaction, EditLog, partition-visible-version or delete-bitmap logic changes. Standard global-variable persistence handles the annotated setting. |
| Data writes and crash cleanup | No production write/commit path changes. LIMIT/cancellation and shared-state destruction retain existing ownership; sole-sink output does not transfer ownership. |
| FE/BE variable passage | Validated setting is captured once, serialized on the sole bucketed-node path, read under __isset, and validated before bucket allocation. Constant-folding/point-query expression paths do not construct this node. |
| Performance and memory | Serial execution avoids scatter and unused COUNT places; active-count loops reduce table overhead. PHHashMap's pinned phmap 1.3.8 implementation mixes raw hashers before top-byte routing, including CRC32; source rehash uses the same interface. Existing task memory context/reservation remains in use. Throughput and final binary behavior were not measured. |
| Other/core invariants | No new data-visibility/version/MoW path. Error status from initialization propagates; nullable const keys are materialized before extraction; no verified new correctness or memory-ownership defect found. |
Existing review context: comment 4344717092 is fixed by disabling both spill flags before the matrix's plan assertions. It is not reposted. No existing P0/P1 inline finding remains to carry. No additional user focus was supplied.
Coverage limits: no builds, unit/regression runs, cluster execution, clang-tidy, final ELF ABI audit or performance benchmarks were attempted, as required by the static-only contract. Golden-file generation provenance and the external session-variable documentation update are not verified. These are validation/documentation limits; no unresolved code candidate remains in the reviewed scope.
Review converged after 1 round(s).
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69425 Problem Summary: The merge-cleanup test calls get_block once and expects an injected exception on a duplicate key. Its original input also contains unique keys, so a changed bucket order can output a unique-key bucket before reaching the merge that throws. Use identical keys in both sinks so every non-empty bucket exercises duplicate merging before output. The test still verifies that all six aggregate states remain available for cleanup after the exception. ### Release note None ### Check List (For Author) - Test: Manual test: reviewed bucket traversal and the failure-before-transfer invariant; clang-format 16, check-format and git diff --check passed. - Unit Test: attempted run-be-ut.sh -j 48 --run --filter=BucketedAggOperatorTest.*; configuration completed, but the build stopped because local libsimdutf.a is missing. Tests did not execute locally and require CI validation. - Behavior changed: No; unit-test data only. - Does this need documentation: No.
|
/review |
|
run buildall |
There was a problem hiding this comment.
Static review of #69425 at 93f2199: no new substantiated findings. The existing P2 spill-configuration comment (4344717092) is fixed by disabling both spill flags before the regression plan assertions. There are no independently confirmed existing P0/P1 blockers to carry forward. No additional user focus was supplied.
The review converged after two rounds covering all 16 changed files, with separate BE runtime, FE/protocol/tests, and fast-path risk reviews. No unresolved candidates or uncovered changed files remain within the static review scope.
Execution flow checked: validated/forwarded session setting -> captured BucketedAggregationNode -> optional Thrift field -> once-only shared initialization -> independent sink tables -> sink completion publication -> per-bucket source merge/output -> NULL-key finalization -> shared-state cleanup.
Critical checkpoint conclusions:
- Goal and proof: automatic/explicit active bucket counts and the single-bucket insertion path are wired end to end. New configuration, plan-capture, shared-state and regression tests cover the intended behavior in source; their runtime success was not verified in this invocation.
- Scope and reuse: changes stay focused on bucketed aggregation and its plumbing/tests; the fast path reuses lazy_emplace_batch and existing validation/forwarding machinery.
- Concurrency and locking: call_once publishes shared configuration; sinks retain separate tables; existing completion release/acquire and per-bucket CAS ownership still govern merging/output. No new lock-order or blocking-under-lock issue identified.
- Lifecycle and initialization: query-owned tables/arenas retain transferred keys and aggregate states through output and teardown. Single-sink merge bypass preserves ownership. No new global initialization or ownership cycle.
- Configuration: 0 or powers of two through 256 are validated before assignment/allocation, forwarded between FEs, and captured per plan; subsequent session changes require no restart and do not mutate an existing node.
- Compatibility: field 6 is optional; its absence retains 256 on the new BE, while older BEs ignore it. Bucket layout is local execution state, with no new persisted or exchanged layout.
- Thrift IDs: checked the complete touched definition on HEAD/current master and current branch-3.1/4.0/4.1/4.2. Field 6 is unused on master; the struct is absent on those release branches. No collision or new required field.
- Parallel and special paths: active-count allocation, routing, reservation, source traversal/completion and NULL merging agree. Numeric/fixed/string/serialized, nullable and materialized constant keys preserve existing key ownership; COUNT updates the actual mapped slot without reading places.
- Conditional checks: range/power-of-two validation, the sink/source cardinality guard and single-sink bypass have concrete documented purposes. Source dependencies are created before task opens; forced one bucket with several sources keeps output serialized by the existing CAS.
- Test coverage: inspected valid/invalid settings, forwarding, plan capture, all valid shared-state counts, concurrent initialization, failed-merge cleanup, nullable/count/general aggregates, high cardinality, multiple batches, empty input and LIMIT. The regression matrix compares against independently checked regular aggregation plans.
- Expected results: independently recomputed all 32768 input rows and checked every value and row multiplicity in all 120 supplied golden sections; all match. Automatic-generation provenance was not established from static files.
- Observability: NumBuckets, NumSinkInstances, PartitionTime and EXPLAIN expose the new decisions; no additional critical diagnostic gap identified.
- Transactions and persistence: no transaction, EditLog, visible-version, delete-bitmap or storage-format change.
- Data writes and failure cleanup: no new data-write path. Existing LIMIT/cancellation owner release, source wakeups and shared-state destruction remain applicable; the merge-failure test is now independent of bucket routing.
- FE/BE propagation: the sole bucketed-node constructor captures the session count; serialization, missing-field behavior and BE validation cover the execution path. No separate point-query/constant-folding bucketed-node send path was found.
- Performance and memory: fewer active tables avoid allocation/scanning overhead; the one-bucket path avoids partitioning and inline COUNT avoids places storage. Reservation scales with the active count; allocation tracking/ownership patterns are preserved. Hash routing consumes phmap-mixed hashes, including CRC32 hashers, as confirmed in the pinned phmap v1.3.8 implementation. No throughput claim was validated by benchmarks.
- Additional review: rechecked hash mixing, creator/reference semantics, NULL-key ownership, source task/dependency setup, immutable plan capture and the existing spill finding; no further actionable issue identified.
Validation limits: static inspection and independent expected-result arithmetic only. No Doris builds, unit/regression execution, runtime concurrency validation or performance benchmarks were performed, as required by the review contract. These limits are separate from convergence of the static review.
Review converged after 2 round(s).
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary:
Bucketed aggregation always allocates 256 hash tables per sink, even when a query has only one sink and one source. It also selects a bucket from mixed-hash bits 24–31. Once a phmap table reaches capacity
2^18 - 1, bit 24 also participates in its probe index, so fixing that bit for a bucket confines initial probes to half the index space.This change adds
bucketed_agg_num_buckets:0selects one bucket when both sink and source have one instance, and 256 otherwise; explicit values must be powers of two from 1 through 256. The planner captures the value and sends it in an optional Thrift field. A missing field preserves 256 buckets for older FEs.Bucket routing uses the high byte of the mixed hash, independently of phmap's practical probe-index and fingerprint bits. The single-bucket path reuses
lazy_emplace_batch, a single sink skips source-side merging, and inline COUNT avoids allocating an unused places buffer. Initialization, memory estimates, source traversal, completion and NULL-key merging all honor the active count. Profiles exposeNumBuckets,NumSinkInstancesandPartitionTime.The regression matrix explicitly disables both spill flags so fuzzy sessions cannot bypass the bucketed plan it requires. The existing subquery translation unit test now uses the planner's actual
CascadesContext, preserving its session variables instead of constructing a context without a session.The merge-failure cleanup unit test uses the same keys in both sinks. Every non-empty bucket therefore contains a duplicate and reaches the injected merge failure before any output, independently of hash routing or bucket traversal order.
Release note
Add the
bucketed_agg_num_bucketssession variable to select an automatic or explicit bucket count for bucketed aggregation. The default automatic mode uses one bucket for serial execution and 256 otherwise. Invalid counts are rejected. SQL aggregation semantics are unchanged.Check List (For Author)
run-fe-ut.sh --run 'org.apache.doris.nereids.rules.analysis.AnalyzeWhereSubqueryTest,org.apache.doris.nereids.glue.translator.BucketedAggregateTranslatorTest,org.apache.doris.qe.SessionVariableBucketedAggTest'— 50 passed, zero failures/errors/skips (15 + 13 + 22).git diff --checkpassed..outfile is unchanged.-j 48, but the build stopped because local third-party artifacts are incomplete (libsimdutf.a;libpaimon_c.ais also absent). The tests did not execute.bucketed_agg_num_buckets; its inline description is included here, and a website documentation PR is still pending.Review checkpoints
Execution flow: session variable → captured plan value → optional Thrift field → once-only shared initialization → independent sink tables → completion publication → per-bucket merge/output → shared-state cleanup.
call_once, completion release/acquire operations and per-bucket CAS ownership are preserved; no new lock order.Check List (For Reviewer who merge this PR)