cetra3 opened a new pull request, #27391: URL: https://github.com/apache/datafusion/pull/27391
> Claude (an AI agent) is raising this PR on behalf of @cetra3. ## Which issue does this PR close? - Closes #27390. ## Rationale for this change `first_value` and `last_value` over a struct, list or map that contains a `Utf8View` or `BinaryView` value take time quadratic in the number of groups. The repro in #27390 takes 5.9 s for 32,000 groups on `main`. The same query with a `Utf8` field takes 24 ms. `GenericValueState::update` compacts each stored value, so each value owns one data buffer. `GenericValueState::take` then concatenates these values, and `concat` keeps every input buffer. So the state array for N groups has N data buffers. In the final stage, `update` compacts each merged row, and that copy clones the list of all N buffers of the source array. So a merge of N groups costs O(N^2). ## What changes are included in this PR? - `GenericValueState::take` compacts the view buffers of the array it builds. It does this only when the data type contains `Utf8View` or `BinaryView`. The state records this once in `new`, so other types pay one boolean check per emit. - Every value in the array already owns its bytes, so the compacted array is not larger than the values. The `gc` inflation that can occur for views that share bytes does not apply here. - `ScalarValue::compact_view_buffers` becomes `#[doc(hidden)] pub` so that `functions-aggregate` can call it. Its doc now says that `gc` can grow an array whose views share bytes. ## What is the testing strategy for this PR? - `test_generic_value_state_struct_utf8view_take_has_one_buffer` updates 50 groups with structs that hold `Utf8View` values longer than 12 bytes, then calls `take`. It checks the values, that the output has one data buffer, and that the buffer holds only the value bytes. Without the fix, the output has 50 buffers and the test fails. - The existing `functions-aggregate` tests pass. ### Benchmarks (local, `main` vs this PR) The #27390 repro with 32,000 groups goes from 5.9 s to 30 ms. No existing benchmark uses a nested value that contains a view type, so none of them runs the new compaction. The existing `first_last` nested benchmarks (struct, list, map and list of struct, for `first_value` and `last_value`) stay within noise. Two cases moved by more than 6% in one run: `evaluate_bench list<i64>[4] nulls=0%` and `evaluate_bench struct(i64,utf8,f64) nulls=90%`. Two back-to-back reruns of `main` and this PR put every `evaluate_bench` case between −4.8% and +1.0%, with no consistent direction. An A/A run of the same code varied by ±3%. ## Are there any user-facing changes? Faster `first_value` / `last_value` on nested values that contain view types. There are no public API changes. `ScalarValue::compact_view_buffers` is public only for internal use and is hidden from the docs. https://claude.ai/code/session_01USkuh3dGhDnDQT3F2Mpdpv -- 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]
