Skip to content

Fix retained memory accounting for ExprIntervalGraph - #26487

Open
kosiew wants to merge 3 commits into
apache:mainfrom
kosiew:memcalc-15-23393
Open

kosiew wants to merge 3 commits into
apache:mainfrom
kosiew:memcalc-15-23393

Conversation

@kosiew

@kosiew kosiew commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Symmetric hash joins use memory reservations to track retained execution state. Previously, ExprIntervalGraph::size() estimated memory using live node and edge counts, which could underreport memory retained by the graph after nodes or edges were removed. It also omitted heap allocations owned by interval bounds.

This change improves reservation accuracy by accounting for retained graph backing capacity and interval-owned heap allocations, while excluding shared physical-expression allocations.

What changes are included in this PR?

  • Replace live node and edge count estimates with capacity-based accounting using the underlying petgraph::StableGraph node and edge storage types.
  • Include heap allocations owned by interval bounds using DFHeapSize and a shared DFHeapSizeCtx to avoid double-counting shared allocations.
  • Exclude shared Arc<dyn PhysicalExpr> pointees from graph-owned memory accounting.
  • Add regression coverage verifying that SymmetricHashJoinStream reservations reflect the graph's reported size after construction, interval updates, and removal.

Are these changes tested?

Three regression tests are added:

  • graph_size_counts_retained_capacity: Verifies empty and grown graph accounting, including retained capacity after node removal and graph clearing.
  • graph_size_counts_interval_heaps_not_expression_pointees: Verifies UTF-8 interval heap accounting, deduplication of shared timestamp timezone allocations, and exclusion of shared physical-expression pointees.
  • stream_reserves_retained_interval_graph: Verifies that stream reservations and memory metrics account for the retained interval graph and are updated when the graph changes or is removed.

The tests use deterministic capacity and size assertions without relying on global allocator measurements.

Are there any user-facing changes?

No changes to query results or public APIs are intended. Memory reservations for symmetric hash joins may now more accurately reflect retained interval graph allocations, potentially affecting memory-limit enforcement.

No documentation changes are expected.

LLM-generated code disclosure

This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.

- ExprIntervalGraph::size() now charges retained capacity + deduplicated interval heaps, excluding expression pointees.
- Added capacity, heap-sharing, stream-reservation tests.
- Graph removal restores reservation + memory metric.
- Timezone counted until final graph reference removed.
@github-actions github-actions Bot added physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate labels Oct 19, 2026
- Captured node handles in cp_solver.rs
- Removed hard-coded indices
- Removed comment
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.26984% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.89%. Comparing base (fe612e2) to head (59148b1).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ion/physical-plan/src/joins/symmetric_hash_join.rs 73.33% 0 Missing and 8 partials ⚠️
...atafusion/physical-expr/src/intervals/cp_solver.rs 96.87% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #26487      +/-   ##
==========================================
- Coverage   82.89%   82.89%   -0.01%     
==========================================
  Files        1154     1154              
  Lines      458022   458179     +157     
  Branches   458022   458179     +157     
==========================================
+ Hits       379683   379811     +128     
- Misses      55303    55315      +12     
- Partials    23036    23053      +17     

☔ 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.

@kosiew
kosiew marked this pull request as ready for review October 19, 2026 04:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants