Skip to content

Fixed CI - #3

Closed
jorgecarleitao wants to merge 4 commits into
masterfrom
fix-ci
Closed

Fixed CI#3
jorgecarleitao wants to merge 4 commits into
masterfrom
fix-ci

Conversation

@jorgecarleitao

Copy link
Copy Markdown
Member
  • Keeps RAT check
  • Keeps all DataFusion and Ballista CI
  • Hacks Archery to run from this repo

@github-actions

Copy link
Copy Markdown

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?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@jorgecarleitao

Copy link
Copy Markdown
Member Author

I will close this and move it to be PRed from my fork, to start aligning processes.

@jorgecarleitao
jorgecarleitao deleted the fix-ci branch April 18, 2021 19:20
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
matthewmturner referenced this pull request in matthewmturner/arrow-datafusion Sep 21, 2021
# 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
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
* Update MD

* Update asf.yaml

* Cleaner name

* keep protected branch

* Remove protected branches for now

* Put `protected_branches` back

* Remove all protected branches

* Remove website + publish
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant