Skip to content

chore: adjust size accounting for min_max - #23899

Merged
comphead merged 1 commit into
apache:mainfrom
comphead:mem_size
Jul 26, 2026
Merged

chore: adjust size accounting for min_max#23899
comphead merged 1 commit into
apache:mainfrom
comphead:mem_size

Conversation

@comphead

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • Part of feat: analyze memory usage for size functions #23393 .

    Rationale for this change

    SlidingMinAccumulator::size and SlidingMaxAccumulator::size only reported
    the stack size of their ScalarValue field plus its heap payload, ignoring the
    memory held by the underlying MovingMin / MovingMax sliding-window buffers.
    For windowed MIN/MAX over string or list data, the two per-element stacks
    can hold megabytes of ScalarValue payload that the memory pool was never
    told about, understating accumulator memory usage.

    What changes are included in this PR?

    • Add a private heap_size(elem_heap) method to MovingMin<T> and MovingMax<T>
      that reports the two stack buffers' capacity in bytes plus each stored
      element's heap payload.
    • Factor the shared implementation into a moving_stacks_heap_size free helper
      so the two types stay in sync.
    • Include the buffer bytes in SlidingMinAccumulator::size and
      SlidingMaxAccumulator::size via the new method.

    Are these changes tested?

    Yes. Two new unit tests in datafusion/functions-aggregate/src/min_max.rs:

    • moving_min_max_heap_size_i32 — fixed-width T, verifies buffer-only
      accounting with and without pushed elements.
    • moving_min_max_heap_size_counts_elemsT = String, verifies each of the
      two slots in a (T, T) pair contributes independently to the heap payload
      (mirroring the two independent Clones made by push).

@github-actions github-actions Bot added the functions Changes to functions implementation label Jul 25, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.75510% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.65%. Comparing base (8393fd3) to head (1afc8ef).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/functions-aggregate/src/min_max.rs 87.75% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #23899   +/-   ##
=======================================
  Coverage   80.65%   80.65%           
=======================================
  Files        1092     1092           
  Lines      371106   371188   +82     
  Branches   371106   371188   +82     
=======================================
+ Hits       299323   299396   +73     
+ Misses      53937    53933    -4     
- Partials    17846    17859   +13     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@comphead

Copy link
Copy Markdown
Contributor Author

Thanks @Jefffrey for the review!

@comphead
comphead added this pull request to the merge queue Jul 26, 2026
Merged via the queue into apache:main with commit 88365dd Jul 26, 2026
61 of 62 checks passed
@comphead
comphead deleted the mem_size branch July 26, 2026 03:42
kosiew pushed a commit to kosiew/datafusion that referenced this pull request Aug 12, 2026
## Which issue does this PR close?

<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax. For example
`Closes apache#123` indicates that this PR will close issue apache#123.
-->

- Part of apache#23393 .

   ## Rationale for this change
  
`SlidingMinAccumulator::size` and `SlidingMaxAccumulator::size` only
reported
the stack size of their `ScalarValue` field plus its heap payload,
ignoring the
memory held by the underlying `MovingMin` / `MovingMax` sliding-window
buffers.
For windowed `MIN`/`MAX` over string or list data, the two per-element
stacks
can hold megabytes of `ScalarValue` payload that the memory pool was
never
  told about, understating accumulator memory usage.

  ## What changes are included in this PR?

- Add a private `heap_size(elem_heap)` method to `MovingMin<T>` and
`MovingMax<T>`
that reports the two stack buffers' capacity in bytes plus each stored
    element's heap payload.
- Factor the shared implementation into a `moving_stacks_heap_size` free
helper
    so the two types stay in sync.
  - Include the buffer bytes in `SlidingMinAccumulator::size` and
    `SlidingMaxAccumulator::size` via the new method.

  ## Are these changes tested?

Yes. Two new unit tests in
`datafusion/functions-aggregate/src/min_max.rs`:

- `moving_min_max_heap_size_i32` — fixed-width `T`, verifies buffer-only
    accounting with and without pushed elements.
- `moving_min_max_heap_size_counts_elems` — `T = String`, verifies each
of the
two slots in a `(T, T)` pair contributes independently to the heap
payload
    (mirroring the two independent `Clone`s made by `push`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants