andygrove commented on PR #5803: URL: https://github.com/apache/datafusion-comet/pull/5803#issuecomment-5607825930
Good catch, and the reasoning is right: the arena is aggregate-wide, so nothing about a group's own result or the combined `List<i32>` element count bounds it, because that limit counts elements rather than encoded bytes. Fixed in e678b4168. `SetEntry.start` is now `u64` and the range is computed once in a small `entry_range` helper with a checked widening and a checked add, so probing, `entry_bytes` and `compact` all go through the same arithmetic instead of each writing `entry.start as usize..(entry.start + entry.len) as usize`. Widening it costs nothing, which is worth stating because it was the thing I checked before choosing `u64` over a segmented representation. Reordering the fields to `hash, start, group, len` keeps `SetEntry` at three machine words, the same 24 bytes it occupied when `start` was 32-bit and the struct carried four bytes of tail padding. `set_entry_stays_three_words` pins that, so a later reorder that reintroduces padding, and therefore charges every distinct value in the aggregate eight more bytes through `size()`, fails rather than passing quietly. `len` stays 32-bit, since one row-encoded Spark value cannot approach 4 GiB, but the narrowing is now checked in `checked_entry_len` and errors instead of truncating. A truncated length is the worse failure of the two: it addresses a shorter slice that is still in bounds, so `insert` would compare against part of a value and `evaluate` would emit it. For the boundary validation you asked for, the arithmetic is tested directly rather than through a real accumulator, so the 4 GiB case is covered without allocating a 4 GiB arena: ```rust assert_eq!(entry_range(u32::MAX as u64 - 4, 8), 4_294_967_291..4_294_967_299); assert_eq!(entry_range(FOUR_GIB, 8), 4_294_967_296..4_294_967_304); assert_eq!(entry_range(FOUR_GIB + 16, 0).start, 4_294_967_312); ``` The first line is the one that mattered: under the old pair `start + len` wrapped to 3, so the slice was `4294967291..3` and panicked. `oversized_entry_len_is_rejected_rather_than_truncated` covers the length guard at `MAX_ENTRY_LEN` and one past it, and `entry_bytes_round_trip_through_the_arena` keeps the ordinary path honest, including that a duplicate does not append a second copy. All 19 `agg_funcs::collect` tests pass, `cargo clippy -p datafusion-comet-spark-expr --all-targets -- -D warnings` is clean, and `cargo fmt --all -- --check` is clean. I did not take the segmented route. It would keep each stored offset small, but it costs a segment index on every probe and every emit for a bound that a single `u64` already covers on any target Comet runs on, and the field-order result means the wider offset is free. -- 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]
