viirya commented on PR #25584: URL: https://github.com/apache/datafusion/pull/25584#issuecomment-5841574077
I agree with @jayzhan211's direction; a few points in addition: - **Memory pressure behaviour regresses.** Representatives are charged to a non-spillable reservation and a failed `try_grow` returns `ResourcesExhausted`, whereas the existing filtered path spills the inner group and completes. `oversized_string_representative_returns_memory_error_and_releases_pool` shows exactly this: the same query that the pairwise path would finish via spilling now fails once the option is on. Reusing `buffer_inner_key_group` (as suggested above) removes this, since the summary is computed from already-accounted, spillable data. If the streaming shape stays, this should at least be documented on the config option. - **Per-row scalar work.** `<>` calls `ScalarValue::eq_array` per row and the range path goes through a `make_comparator` closure per row. With the min/max formulation (`x <> min OR x <> max`) both become `min_batch`/`max_batch` over the (guard-filtered) column plus one vectorized comparison, and `State::set_multiple`/the overlap accounting go away. - **Pool traffic per key group.** Each group does `try_grow` + `shrink` per clause and a `free` on reset. Under `FairSpillPool` these take a mutex, so workloads with many tiny groups across partitions pay contention the 4096-key benchmark wouldn't show. For fixed-width types the size is known up front and could be reserved once. - `peak_mem_used` changes meaning on this path (representatives only). I'd rather keep it consistent with the other paths, or not set it here. - Nit: `scalar_storage_size` charges the timestamp time zone length, but `ScalarValue` shares the `Arc<str>` from the `DataType`, so nothing is allocated. -- 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]
