cetra3 commented on PR #25800: URL: https://github.com/apache/datafusion/pull/25800#issuecomment-6458773251
I'm a bit concerned about `repeated_view_buffers_size`. We already have 5 different ways of measuring how much memory a view array holds, and this PR adds a 6th: | # | Mechanism | View data buffers | Main users | |---|---|---|---| | 1 | Arrow `get_array_memory_size()` | Full capacity of every buffer entry; repeats and sharing across columns or batches counted again | ~60 non-test call sites: symmetric hash join, repartition, aggregate streams, `spill_pool`, `buffer.rs`, NLJ, SMJ, `materialized_cte`, `DFHeapSize` | | 2 | Arrow `ArrayData::get_slice_memory_size()` | Proportional to the slice for fixed buffers, full capacity per variadic entry, repeats counted again | `GetSlicedSize`, `array_agg` `size()`, hash join fallback | | 3 | `get_record_batch_memory_size` / `RecordBatchMemoryCounter` / `RecordBatchMemoryTracker` (`common/src/utils/memory.rs`) | Each buffer counted once by allocation address, one visitor (`visit_array_buffers`) for all nested types | sort, TopK, `BatchBuilder`, multi-level merge, hash join, asof / piecewise merge join, baseline metrics | | 4 | `dedup_buffer_entries` + `should_gc_view_array` (`spill/mod.rs`) | Dedup by `(as_ptr, len)` (slice identity), sums `len` rather than `capacity` | Compacting view arrays before spilling | | 5 | `estimate_concat_allocation` (`hash_join/exec.rs`) | Hand-written view case: `len * 16` + null bitmap | Hash join build side | | **6** | **This PR: `repeated_view_buffers_size`** | **Its own `ArrayData` walk and dedup set, subtracted from #2** | `get_sliced_size` | They don't agree on what "the same buffer" means (allocation address vs. slice) or what to count (`capacity` vs. `len`). Part of #25791 is exactly this mismatch: the sort reservation is `get_record_batch_memory_size(batch) + batch.get_sliced_size()`, which is #3 (deduplicated) plus #2 (not deduplicated). #6 also reimplements what #3 already does: walking nested arrays and deduplicating buffers by address. Unless there is a good reason not to use the existing implementations, I think this should build on #3 rather than adding a separate walker in `spill_manager.rs`. I don't think this PR needs to clean up the other call sites. I'd just rather not add another implementation that a later consolidation then has to remove. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
