kosiew commented on PR #25188: URL: https://github.com/apache/datafusion/pull/25188#issuecomment-5828206850
[8ec5f3e](https://github.com/apache/datafusion/pull/25188/commits/8ec5f3e759fd0e1e19af26661d7b3841a594388b) - `VectorizedOperationBuffers::clear_shrink`: `self.equal_to_results = BooleanBufferBuilder::new(num_rows)` does the same as the `finish`/`append_n`/`truncate` sequence, and the comment can go. [8a477d6](https://github.com/apache/datafusion/pull/25188/commits/8a477d64e529db201cbbba30e996c5f2560d931e) - `DictionaryGroupValuesColumn::sync_value_cache`: the `Weak` keeps the allocation reserved, so comparing addresses is enough and `upgrade()` is not needed: `std::ptr::addr_eq(cached.as_ptr(), Arc::as_ptr(dict_values))`. A small `is_cached(&self, values: &ArrayRef) -> bool` would also replace the four `upgrade().is_some_and(...)` blocks in the tests. [bea282b](https://github.com/apache/datafusion/pull/25188/commits/bea282b1a11e1b50edd1a7b315dfef39ffb72d0a) - `legacy_aggregate_spill_merge_leaves_memory_for_replay`: a literal budget with a short comment would avoid the `pub(crate)` re-export of `GroupValuesPrimitive`, which only this test uses. [4995794](https://github.com/apache/datafusion/pull/25188/commits/49957946883116d05a6ef6ead2ab18d679c1f332) - `aggregate_memory_spill.slt` Case G: is `2M` still needed? On the current head it passes at `1M` for me (5 of 5 runs, `release-nonlto`), and `1M` keeps the pressure this case is meant to exercise. -- 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]
