Repository navigation
Fix retained memory accounting for ExprIntervalGraph - #26487
Merged
Merged
Conversation
- 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
Bot
added
physical-expr
- Captured node handles in cp_solver.rs - Removed hard-coded indices - Removed comment
Codecov Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
kosiew
marked this pull request as ready for review
October 19, 2026 04:33
alamb
approved these changes
Oct 29, 2026
| use rand::{Rng, SeedableRng}; | ||
| use rstest::*; | ||
|
|
||
| fn expected_graph_backing_size(graph: &ExprIntervalGraph) -> usize { |
Contributor
There was a problem hiding this comment.
this is a lot of lines of test -- I wonder if there is some way to reduce the replication / make some fixtures?
Contributor
Author
There was a problem hiding this comment.
Added shared graph/node setup helpers in cp_solver - 2a77e6d
Contributor
|
run benchmark sql_planner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Runner log (last 40 lines)Kubernetes messageFile an issue against this benchmark runner |
|
Benchmark for this request failed before finishing (Kubernetes reason: Benchmarks requested: Runner log (last 40 lines)Kubernetes messageFile an issue against this benchmark runner |
- Shared graph/node setup helpers; assertions unchanged.
Contributor
Author
|
🚀 |
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.
Which issue does this PR close?
sizefunctions #23393Rationale 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?
petgraph::StableGraphnode and edge storage types.DFHeapSizeand a sharedDFHeapSizeCtxto avoid double-counting shared allocations.Arc<dyn PhysicalExpr>pointees from graph-owned memory accounting.SymmetricHashJoinStreamreservations 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.