Fixed CI - #3
Closed
jorgecarleitao wants to merge 4 commits into
Closed
Conversation
jorgecarleitao
commented
Apr 18, 2021
Member
- Keeps RAT check
- Keeps all DataFusion and Ballista CI
- Hacks Archery to run from this repo
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename pull request title in the following format? or See also: |
Member
Author
|
I will close this and move it to be PRed from my fork, to start aligning processes. |
yjshen
referenced
this pull request
in yjshen/datafusion
Sep 17, 2021
* wip * more * Make scalar.rs compile * Fix various compilation error due to API difference * Make datafusion core compile * fmt * wip
alamb
added a commit
that referenced
this pull request
Sep 22, 2021
* # This is a combination of 3 commits. # This is the 1st commit message: Add Display for Expr::BinaryExpr # This is the commit message #2: Update logical_plan/operators tests # This is the commit message #3: rebase and debug display for non binary expr * Add Display for Expr::BinaryExpr Update logical_plan/operators tests rebase and debug display for non binary expr Add Display for Expr::BinaryExpr Update logical_plan/operators tests Updating tests Update aggregate display Updating tests without aggregate More tests Working on agg/scalar functions Fix binary_expr in create_name function and attendant tests More tests More tests Doc tests Rebase and update new tests * Submodule update * Restore submodule references from master Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
EricJoy2048
added a commit
to EricJoy2048/arrow-datafusion
that referenced
this pull request
Mar 2, 2022
andygrove
added a commit
that referenced
this pull request
Jan 12, 2023
* Initial commit * initial commit * failing test * table scan projection * closer * test passes, with some hacks * use DataFrame (#2) * update README * update dependency * code cleanup (#3) * Add support for Filter operator and BinaryOp expressions (#4) * GitHub action (#5) * Split code into producer and consumer modules (#6) * Support more functions and scalar types (#7) * Use substrait 0.1 and datafusion 8.0 (#8) * use substrait 0.1 * use datafusion 8.0 * update datafusion to 10.0 and substrait to 0.2 (#11) * Add basic join support (#12) * Added fetch support (#23) Added fetch to consumer Added limit to producer Added unit tests for limit Added roundtrip_fill_none() for testing when None input can be converted to 0 Update src/consumer.rs Co-authored-by: Andy Grove <andygrove73@gmail.com> Co-authored-by: Andy Grove <andygrove73@gmail.com> * Upgrade to DataFusion 13.0.0 (#25) * Add sort consumer and producer (#24) Add consumer Add producer and test Modified error string * Add serializer/deserializer (#26) * Add plan and function extension support (#27) * Add plan and function extension support * Removed unwraps * Implement GROUP BY (#28) * Add consumer, producer and tests for aggregate relation Change function extension registration from absolute to relative anchor (reference) Remove operator to/from reference * Fixed function registration bug * Add test * Addressed PR comments * Changed field reference from mask to direct reference (#29) * Changed field reference from masked reference to direct reference * Handle unsupported case (struct with child) * Handle SubqueryAlias (#30) Fixed aggregate function register bug * Add support for SELECT DISTINCT (#31) Add test case * Implement BETWEEN (#32) * Add case (#33) * Implement CASE WHEN * Add more case to test * Addressed comments * feat: support explicit catalog/schema names in ReadRel (#34) * feat: support explicit catalog/schema names in ReadRel Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * fix: use re-exported expr crate Signed-off-by: Ruihang Xia <waynestxia@gmail.com> Signed-off-by: Ruihang Xia <waynestxia@gmail.com> * move files to subfolder * RAT * remove rust.yaml * revert .gitignore changes * tomlfmt * tomlfmt Signed-off-by: Ruihang Xia <waynestxia@gmail.com> Co-authored-by: Daniël Heres <danielheres@gmail.com> Co-authored-by: JanKaul <jankaul@mailbox.org> Co-authored-by: nseekhao <37189615+nseekhao@users.noreply.github.com> Co-authored-by: Ruihang Xia <waynestxia@gmail.com>
alamb
added a commit
that referenced
this pull request
Feb 10, 2025
* add mut annotation * fix rust examples * fix rust examples * update * fix first doctest * fix first doctest * fix more doctest * fix more doctest * fix more doctest * adopt rustdoc syntax * adopt rustdoc syntax * adopt rustdoc syntax * fix more doctest * add missing imports * final udtf * reenable * remove dep * run prettier * api-health * update doc * update doc * temp fix * fix doc * fix async schema provider * fix async schema provider * fix doc * fix doc * reorder * refactor * s * finish * minor update * add missing docs * add deps (#3) * fix doctest * update doc * fix doctest * fix doctest * tweak showkeys * fix doctest * fix doctest * fix doctest * fix doctest * update to use user_doc * add rustdoc preprocessing * fix dir * revert to original doc * add allocator * mark type * update * fix doctest * add doctest * add doctest * fix doctest * fix doctest * fix doctest * fix doctest * fix doctest * fix doctest * fix doctest * fix doctest * fix doctest * prettier format * revert change to datafusion-testing * add apache header * install cmake in setup-builder for ci workflow dependency * taplo + fix snmalloc * Update function docs * preprocess user-guide * Render examples as sql * fix intro * fix docs via script --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
goldmedal
added a commit
that referenced
this pull request
Jul 31, 2025
… default null ordering (#16963) * Allow to set up the default null ordering (#3) * add default_null_ordering config * add test for different config * Update datafusion/sql/src/planner.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * update doc * fix sqllogictest --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * rename config and fix test * update doc * fix default * fix doc * fix sqllogictests * address comments --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Standing-Man
pushed a commit
to Standing-Man/datafusion
that referenced
this pull request
Aug 4, 2025
… default null ordering (apache#16963) * Allow to set up the default null ordering (apache#3) * add default_null_ordering config * add test for different config * Update datafusion/sql/src/planner.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * update doc * fix sqllogictest --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * rename config and fix test * update doc * fix default * fix doc * fix sqllogictests * address comments --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
blaginin
added a commit
that referenced
this pull request
Nov 12, 2025
This was referenced May 13, 2026
mkleen
pushed a commit
to mkleen/datafusion
that referenced
this pull request
Jul 15, 2026
## Which issue does this PR close? - Part of apache#23334. > The numbers below come from the committed criterion benchmark added in apache#23335 (`cargo bench --bench array_has`) — **origin** = the per-row `eq` kernel (unoptimized `main` / apache#23335), **now** = with this optimization applied. Run the bench on `main` and on this branch to reproduce. Full disclosure - this was heavily assisted by AI, and I did my best to understand and justify every change here before submitting. ## Rationale for this change `array_has(array, element)` returns, for each row, whether the array contains the element. When the `element` (needle) is an array rather than a scalar, the needle argument is a column with one value per row, e.g. `array_has(t1.tags, t2.key)` in a join filter, execution goes through `array_has_dispatch_for_array` (the `ColumnarValue::Array` needle branch), which compared each row by invoking the Arrow `eq` kernel once per row. That kernel allocates a `BooleanArray` and pays downcast and dispatch overhead on every row. (The scalar-needle branch was optimized separately in apache#20374.) What this removes is the fixed per-row kernel overhead, not the element comparison itself, so the gain is largest for short lists and shrinks as lists grow. All numbers below are from the committed criterion benchmark (`cargo bench --bench array_has`, groups `array_has_array_null_patterns` / `array_has_array_by_size` / `array_has_array_by_rows`): the `array_has` UDF evaluated in isolation with an array needle, **origin** (the per-row `eq` kernel) vs **now**. "list length" is the number of elements in each row's array (not the row count). Not end-to-end query time. ### By data type and null pattern (list length 64, 10K rows) | element | element len | null pattern | origin | now | speedup | |-----------|----------------|----------------------|---------|---------|---------| | i64 | - | no nulls, found | 1.10 ms | 73 µs | 15.1x | | i64 | - | no nulls, not found | 1.07 ms | 72 µs. | 14.9x | | i64 | - | 30% nulls, found | 1.17 ms | 315 µs | 3.7x | | i64 | - | 30% nulls, not found | 1.10 ms | 274 µs | 4.0x | | i64 | - | all null | 1.10 ms | 272 µs | 4.0x | | i64 | - | collision | 1.10 ms | 270 µs | 4.1x | | Utf8 | short (inline) | no nulls | 2.57 ms | 1.01 ms | 2.5x | | Utf8 | short (inline) | 30% nulls | 3.37 ms | 1.52 ms | 2.2x | | Utf8 | long (>12B) | no nulls | 2.61 ms | 1.04 ms | 2.5x | | Utf8 | long (>12B) | 30% nulls | 3.31 ms | 1.52 ms | 2.2x | | Utf8 | - | all null | 1.26 ms | 256 µs | 4.9x | | LargeUtf8 | short (inline) | no nulls | 2.56 ms | 1.02 ms | 2.5x | | LargeUtf8 | short (inline) | 30% nulls | 3.20 ms | 1.54 ms | 2.1x | | LargeUtf8 | long (>12B) | no nulls | 2.67 ms | 1.05 ms | 2.6x | | LargeUtf8 | long (>12B) | 30% nulls | 3.42 ms | 1.59 ms | 2.2x | | LargeUtf8 | - | all null | 1.31 ms | 263 µs | 5.0x | | Utf8View | short (inline) | no nulls | 1.18 ms | 239 µs | 4.9x | | Utf8View | short (inline) | 30% nulls | 1.26 ms | 246 µs | 5.1x | | Utf8View | long (>12B) | no nulls | 2.86 ms | 1.17 ms | 2.4x | | Utf8View | long (>12B) | 30% nulls | 3.51 ms | 1.66 ms | 2.1x | | Utf8View | - | all null | 1.20 ms | 267 µs | 4.5x | The i64 null cases are uniform (~4x) whether the match is present, absent, the whole list is null, or the needle collides with a null slot's backing fill value — validity is folded in with one word-parallel op, so there is no per-row rescan and no null slot can match. Strings win ~2.1–2.5x mainly by dropping the per-row `BooleanArray` allocation. `Utf8View` additionally uses a view-aware compare: the byte length and 4-byte prefix packed into the 128-bit view reject non-matches before touching the data buffer, and an inline value (≤ 12 bytes) is matched by whole-view equality with no materialization at all — hence ~5x on short/inline strings. When long strings share a prefix (e.g. ARNs) the prefix can't reject, so `Utf8View` falls in line with the other string types (~2.1–2.4x). No string case regresses. ### By list length (i64, 30% element nulls, not found, 10K rows) | elems/row | origin | now | speedup | |-----------|---------|---------|--------------------------------------| | 8 | 1.03 ms | 111 µs | 9.3x | | 32 | 1.07 ms | 197 µs | 5.5x | | 128 | 1.18 ms | 446 µs | 2.6x | | 256 | 1.28 ms | 780 µs | 1.6x | | 512 | 1.54 ms | 1.44 ms | 1.1x | | 1024 | 2.17 ms | 2.15 ms | 1.0x (falls back to per-row kernel) | The element-null branch makes a few passes over the values; past a moderate average list length (`NULL_FAST_PATH_MAX_LEN`) the per-row kernel wins, so it bails to it there — no meaningful regression. That average is measured over the visible (sliced) region, so a sliced array's hidden child elements can't route a small window to the slow path. The all-valid fold has no such crossover. ### By row count (i64, 8 elems/row, 30% nulls, not found) | rows | origin | now | speedup | |------|-----------|----------|---------| | 10K | 1.04 ms | 111 µs | 9.4x | | 100K | 10.42 ms | 1.09 ms | 9.6x | | 1M | 102.68 ms | 10.91 ms | 9.4x | Invariant to the number of rows — the per-row overhead removed is a fixed cost, so absolute savings scale linearly with the column height. The remaining benchmarks in the suite (scalar `array_has`, `array_has_all`, `array_has_any` — paths this PR does not touch) are unchanged (median 0.99x, within measurement noise), confirming no regression outside the array-needle path. ### End-to-end (context) For a query dominated by an array-needle `array_has` join filter (a `NestedLoopJoinExec` with `filter=array_has(tags, key)` over 3000x3000 rows of 8-element lists) total time drops from 0.95s to 0.059s (~16x, identical results). For a workload where `array_has` is a smaller fraction, e.g. the ~6% of profile that motivated this (see apache#18070 / apache#18161, which fixed the join's deep-copy but left the per-row `array_has` cost), the overall speedup is single-digit percent. ## What changes are included in this PR? A fast path for primitive and string element types in `array_has_dispatch_for_array`, preserving the Arrow `eq` kernel semantics (total-order float equality; null elements never match): - **All-valid elements:** each row is a single branchless OR-reduction over the raw native value slice (auto-vectorizes; the common case). - **Element nulls:** a null slot's backing value is arbitrary, so the per-element equality bitmap is ANDed with the validity bitmap (one word-parallel op, no per-element branch) before reducing each row to "any bit set", a null slot can never match regardless of its value. This branch is processed in row chunks so the scratch buffer stays bounded, and past `NULL_FAST_PATH_MAX_LEN` average elements/row a length check over the visible (sliced) region bails to the per-row kernel (see the list-length table). - **String elements:** each row is a single pass over the row's values (compare, then consult validity only on a match). `Utf8View` compares the packed 128-bit views directly — length + 4-byte prefix reject non-matches before any data-buffer access, and an inline value (≤ 12 bytes) matches by whole-view equality with no materialization. - **Nested (and any other) element types** keep using the per-row `eq` kernel. The array-needle benchmarks used for the numbers above are added in apache#3 (null patterns, list length, and row count). ## Are these changes tested? Yes: - New unit tests for the array-needle path covering element nulls, the null-fill collision (needle equal to a null slot's backing value), total-order float equality (`NaN` / `-0.0`), sliced arrays (including a small visible window over a large backing child), `LargeList` offsets, empty rows, a multi-chunk input, and a long-list input that exercises the per-row fallback, each cross-checked against the original per-row `eq` kernel as an oracle. - Existing `array_has` / `array_contains` / `join_lists` sqllogictest suites pass. ## Are there any user-facing changes? No. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
TinyMurky
pushed a commit
to TinyMurky/datafusion
that referenced
this pull request
Sep 7, 2026
…TINCT) group (apache#24857) A grouped `COUNT(DISTINCT <string>)` over 4,000 groups holding 2 short strings each needs a 36 MB memory budget. It needs 2.0 MB after this change. Every group gets its own hash table, and each table is allocated at warm-up size before the group holds anything, so the memory the query needs tracks the number of groups rather than the amount of data. The query also reports less memory than it holds, so a memory limit does not stop it at the right point. ## Reproduction This needs only `datafusion-cli`. There is no patch, no custom allocator and no data file. ```sql -- repro.sql SET datafusion.execution.target_partitions = 1; -- 4,000 groups with 2 distinct short strings in each. -- avg() stops SingleDistinctToGroupBy from rewriting the distinct aggregate away. SELECT g, count(DISTINCT s) AS d, avg(p) AS a FROM ( SELECT v % 4000 AS g, 'v' || CAST(v AS VARCHAR) AS s, v AS p FROM generate_series(0, 7999) AS t(v) ) GROUP BY g ORDER BY g LIMIT 3; ``` ``` datafusion-cli -m 8M -f repro.sql ``` `s` is a `Utf8View` column, so this exercises `ArrowBytesViewMap`. The 8,000 rows arrive in one batch, so the aggregate builds all 4,000 accumulators before it can emit or spill. Current `main` at `20d1c56761` fails: ``` Resources exhausted: Additional allocation failed for SingleHashAggregateStream[0] with top memory consumers (across reservations) as: DataFusion-Cli#1(can spill: false) consumed 0.0 B, peak 0.0 B, SingleHashAggregateStream[0]apache#2(can spill: true) consumed 0.0 B, peak 48.0 B, TopK[0]apache#3(can spill: false) consumed 0.0 B, peak 0.0 B. Error: Failed to allocate additional 111.1 MB for SingleHashAggregateStream[0] with 0.0 B already allocated for this reservation - 8.0 MB remain available for the total memory pool: greedy(used: 0.0 B, pool_size: 8.0 MB) ``` This branch returns the rows: ``` +---+---+--------+ | g | d | a | +---+---+--------+ | 0 | 2 | 2000.0 | | 1 | 2 | 2001.0 | | 2 | 2 | 2002.0 | +---+---+--------+ 3 row(s) fetched. Elapsed 0.006 seconds. ``` Those 4,000 accumulators hold 8,000 short strings, which is about 100 KB of data. The base asks the pool for 111.1 MB to hold it. This branch runs the same query inside `-m 3M`. Both builds return the same rows, and the base does so at `-m 200M`. Each run takes well under a second, and the outcome repeats exactly over three runs on each side. ## How much it improves The minimum memory limit at which that query completes, bisected on each side: | value column | before | after | | --- | --- | --- | | `Utf8` | fails 34 MB, passes 36 MB | fails 1.8 MB, passes 2.0 MB | | `Utf8View` | fails 120 MB, passes 124 MB | fails 2.4 MB, passes 2.6 MB | `clickbench_extended` at `DATAFUSION_RUNTIME_MEMORY_LIMIT: 4G`, pool peak over six runs: | query | base | this branch | change | | --- | --- | --- | --- | | Q2, grouped, 4 string distincts | 98.1 to 98.6 MiB | 11.7 MiB in all six runs | -88.1% | | Q1, ungrouped string distincts | 3.4 MiB | 2.4 MiB | -29.4% | | Q0, ungrouped, high cardinality | 796.8 to 834.8 MiB | 846.5 to 885.5 MiB | +6.3% | Q2 is the only query in any benchmark suite that puts a grouped `COUNT(DISTINCT)` on a non-integer column. Q0 costs more, and it is the one disclosed cost of this PR. Those extra bytes are memory Q0 always held and the pool could not see, not new allocation; appendix A has the three-build decomposition that separates the two. Latency does not move anywhere, which is what an allocation-sizing change should do. ## Which issue does this PR close? No existing issue. I found this when I investigated a production out of memory. I can file an issue if you want it in the changelog. ## Rationale for this change Pre-allocating is right for the one long-lived map behind a `GROUP BY` on a string column. It is wrong for the distinct-count accumulators, because `GroupsAccumulatorAdapter` creates one accumulator per group and most groups hold a handful of values. There the warm-up dwarfs the data. Both maps also under-report the table they hold. `ArrowBytesViewMap` left the control bytes out. `ArrowBytesMap` charged the table only when it grew, so a map that stayed inside its pre-allocation reported its table as free forever. A memory limit acted on a number that was too small. `clear_shrink` is the third part. The aggregate stream calls it to hand memory back before it spills and before a downstream sort. It restored the warm-up capacity instead of releasing it, so nothing came back. ## What changes are included in this PR? - `new` on both maps allocates nothing. A new `with_capacity` keeps the previous pre-allocating behavior. `GroupValuesBytes` and `GroupValuesBytesView` use `with_capacity`, and the two distinct-count accumulators use `new`. A map remembers how it was built, so `take` warms it back up the way it started. - `size()` reports `HashTable::allocation_size()`, the real hashbrown allocation including the control bytes, in place of the old estimate. - A new `clear_and_release` drops every allocation the map holds, and `clear_shrink` calls it. - The value buffer rounds each growth up to a power of two. A lazily grown buffer and a pre-allocated one then sit on one ladder, so a lazy map is never the larger of the two for the same values. Growth stays geometric. - `benches/arrow_bytes_map.rs` moves to `with_capacity` so it keeps measuring the pre-allocating constructor. ## What is the testing strategy for this PR? Two tests in `datafusion/core/tests/memory_limit/mod.rs`, `group_by_count_distinct_utf8` and `group_by_count_distinct_utf8_view`, turn the headline claim into a pass or a fail rather than a number. They run the reproduction query over 4,000 groups with spilling disabled and `target_partitions` pinned to 1, so completing means the query fits the budget rather than spills out of it. The limits are 8 MB and 16 MB, at least 4x clear of both cliffs in the table above. Both tests fail on the merge base and pass here, over five consecutive runs. The `avg(payload)` in the query is load bearing; appendix C says why. Unit tests cover what the memory-limit tests cannot see: that `new` allocates nothing, that `with_capacity` reports a table size bracketed by an independently derived lower bound, that `take` preserves the configured capacity, that `clear_shrink` drops the reported size to near zero, and that a lazily grown buffer never exceeds a pre-allocated one holding the same values. Run locally on the rebased head, all passing: `datafusion-physical-expr-common` (87 lib, 8 doc), `datafusion-functions-aggregate-common` (49), `datafusion-functions-aggregate -- count_distinct` (2), `datafusion-physical-plan -- group_values` (96) and the `memory_limit` module (39, which includes the `count_distinct_spill` test that arrived on `main` in apache#24888 and apache#24918). `cargo fmt --check` and `cargo clippy --all-targets -D warnings` are clean on the changed crates. CI has not yet run this branch against the new base. No query results change. ## Are there any user-facing changes? Yes, in `datafusion-physical-expr-common`. `ArrowBytesMap::new` and `ArrowBytesViewMap::new` no longer pre-allocate, and callers that want the previous behavior should use the new `with_capacity`. Both types also gain `clear_and_release`. This changes an existing public constructor rather than adding one, so tell me if you would like the `api change` label. For users, a grouped `COUNT(DISTINCT)` on string and binary columns uses much less memory and reports its usage to the `MemoryPool` accurately. A query that previously hit a memory limit may now succeed. --- ## Appendix A: Query 0 costs 6.3% more Q0 is `COUNT(DISTINCT)` over three high-cardinality strings with no `GROUP BY`. It is a handful of maps that each grow to millions of entries, which is the opposite population from the one this PR targets. The pre-allocation was never the dominant cost there, so removing it buys nothing. Over six runs the base spans 796.8 to 834.8 MiB and this branch spans 846.5 to 885.5 MiB. The ranges do not overlap, so the effect is real and not run-to-run noise. Three local builds on a deterministic subset separate the two changes. The middle build differs from the base only in the accounting, because restoring the warm-up makes the constructors byte-identical to base: | build | Q0 pool peak | | --- | --- | | base | 48,421,820 | | this branch with the warm-up restored | 51,048,396 | | this branch | 51,018,828 | That decomposes the increase exactly. +2,626,576 is the accounting correction: `allocation_size()` charges the real hashbrown allocation, which is `4 * buckets + 5,384` more than the old formula, being the control bytes plus the 7/8 load-factor slack. -29,568 is the lazy constructor, which makes Q0 slightly better. Reverting the accounting would restore an under-report of about 19% on this path. That under-report is the bug this PR exists to fix, and the memory-limit result above depends on fixing it. ## Appendix B: what one accumulator costs One per-group accumulator holding a single 24-byte value: | | before, actual | before, reported | after | | --- | --- | --- | --- | | `BytesDistinctCountAccumulator` | 14,648 B | 8,240 B | 180 B | | `BytesViewDistinctCountAccumulator` | 33,920 B | 28,792 B | 260 B | The middle column is the reporting gap. The `Utf8` map really held 14,648 bytes and reported 8,240, because the whole hash table was invisible to the old accounting. These are measured directly rather than asserted in a test, since the exact numbers follow the hashbrown layout. ## Appendix C: notes on the tests and the benchmarks The memory-limit query uses `avg(payload)`, not `count(*)`. A non-distinct `count` lets `SingleDistinctToGroupBy` rewrite the distinct aggregate into a plain two-stage `GROUP BY`. The per-group accumulators would then never exist, and the tests would pass by construction on the base commit too. That rule accepts a non-distinct `sum`, `min` or `max` because each re-aggregates its own partial results correctly over the deduplicated inner group by. `avg` does not, so the rule can never admit it under any extension, including the one apache#24859 proposes. The benchmark figures were measured against the previous merge base `da89c7c85b`. They have not been re-run against the current base `20d1c56761`. The commits after `84f07da` on this branch touch only `datafusion/core/tests/memory_limit/mod.rs`, so nothing on this branch since then can move a benchmark, but the base itself has moved. Pool peak is the instrument here, not peak RSS. Pool peak reproduces to under 1% on a 98 MiB query and exactly on the 3.4 and 11.7 MiB ones. Peak RSS on this harness has a 4.1% standard deviation over 11 readings of identical code plus a 2.3% order bias, and shows no effect from this change once that null is accounted for. ## Follow-ups, not in this PR - The same undercount remains at five other production `insert_accounted` call sites: `group_values/row.rs:171`, `multi_group_by/mod.rs:434,554`, `multi_group_by/dictionary.rs:197,584` and `array_agg.rs:989`. Each is one map per query, so the absolute error is bounded, and the fix is the same one-line swap. - The `count_distinct_groups` benchmarks in `datafusion/functions-aggregate/benches/count_distinct.rs` cover `Int64`, `Int32` and `UInt32` only, so this path has no criterion coverage. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.