Repository navigation
feat: Fix output bytes metric in hash agg - #25086
ariel-miculas wants to merge 4 commits into
Conversation
github-actions
Bot
added
physical-expr
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25086 +/- ##
==========================================
+ Coverage 81.71% 81.74% +0.03%
==========================================
Files 1127 1128 +1
Lines 416072 416782 +710
Branches 416072 416782 +710
==========================================
+ Hits 339993 340708 +715
+ Misses 56091 56001 -90
- Partials 19988 20073 +85 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The approach looks good overall, and I don't see anything blocking this PR. I left a couple of suggestions around the SQLLogicTest documentation and coverage of the migrated hash aggregate stream paths.
| # Using `RecordBatch::record_output` (as the streams used to) recomputes | ||
| # memory usage from scratch on every slice, with no memory of buffers | ||
| # already counted -- so all N slices of one materialization each count the | ||
| # shared buffers again, inflating `output_bytes` by ~Nx. `RecordBatchMemoryMetrics` |
There was a problem hiding this comment.
Small documentation nit: the new implementation doesn't use RecordBatchMemoryMetrics, and I don't think that type exists. The fix records the unsliced materialized batch once via BaselineMetrics::record_output_bytes. Could you update this test header to describe the mechanism that's actually being tested?
| /// range, giving many independently allocated emits. `batch_size` 8192 | ||
| /// leaves them whole; `batch_size` 100 slices each 512-row emit into 6. | ||
| #[tokio::test] | ||
| async fn output_bytes_metric_is_exact_for_sliced_and_dropped_batches() -> Result<()> { |
There was a problem hiding this comment.
Could we add some focused metric regression coverage for the migrated hash streams changed by this PR: PartialHashAggregateStream, FinalHashAggregateStream, SingleHashAggregateStream, and PartialReduceHashAggregateStream? The migration flag already defaults to true, but this test provides sorted input, so it selects OrderedPartialAggregateStream rather than PartialHashAggregateStream. It would be useful to use linear input and assert which stream path is selected, or construct the streams directly, while still exercising sliced output. Similar legacy fallback coverage would also be useful while GroupedHashAggregateStream remains supported.
|
Thanks for taking a look, @kosiew |
3 participants
Which issue does this PR close?
Alternative POC to #23777
Rationale for this change
Summing together the produced sliced RecordBatches in hash aggregation leads to inflated output_bytes metric
What changes are included in this PR?
What is the testing strategy for this PR?
sqllogic test and unit tests
Are there any user-facing changes?