Uh oh!
There was an error while loading. Please reload this page.
Add was_valid parameter to NullState callbacks - #11592
Conversation
| /// Check if the accumulated value for the group at the given `index` is valid, | ||
| /// meaning that there was at least one value passing the filter for this group. | ||
| pub fn is_valid(&self, index: usize) -> bool { | ||
| self.seen_values.get_bit(index) | ||
| } |
There was a problem hiding this comment.
Ahh I can't use it in accumulate due to mutable vs immutable borrow. This API is pretty difficult to use.
There was a problem hiding this comment.
I don't understand this comment 🤔
There was a problem hiding this comment.
This is the signature of accumulate:
pubfnaccumulate<T,F>(&mutself,group_indices:&[usize],values:&PrimitiveArray<T>,opt_filter:Option<&BooleanArray>,total_num_groups:usize,mutvalue_fn:F,)whereT:ArrowPrimitiveType + Send,F:FnMut(usize,T::Native) + SendBut I can't use is_null inside value_fn because the NullState is already borrowed as mutable
There was a problem hiding this comment.
Ideally BooleanBufferBuilder::set_bit should return the previous value and we can pass it along to the callback. That's in arrow I guess.
joroKr21
commented
Jul 22, 2024
Yeah good point, I converted it to a draft. I'm not sure what to do. Adding a boolean flag to the callback is not great either... |
This reverts commit a44dd81.
joroKr21
commented
Jul 22, 2024
@alamb I implemented the version with an additional callback parameter, LMK what you think? |
alamb
left a comment
There was a problem hiding this comment.
The code looks good to me @joroKr21
I think this PR needs:
- Update the docs
- A functional test
- Run the clickbench performance benchmarks to ensure we don't see a regression
I can help with the benchmarks if necessary
cc @Dandandan
| |group_index, new_value| { | ||
| let value = &mut self.values[group_index]; | ||
| (self.prim_fn)(value, new_value); | ||
| |group_index, was_valid, new_value| { |
There was a problem hiding this comment.
It would be nice to add a test that covered this, if possible
| ) where | ||
| T: ArrowPrimitiveType + Send, | ||
| F: FnMut(usize, T::Native) + Send, | ||
| F: FnMut(usize, bool, T::Native) + Send, |
There was a problem hiding this comment.
Can we please update the documentation to reflect this new argument and explains what it means
13601c8 to
477eadaComparejoroKr21
commented
Jul 23, 2024
/benchmark |
| |group_index, new_value| { | ||
| let value = &mut self.values[group_index]; | ||
| (self.prim_fn)(value, new_value); | ||
| |group_index, was_valid, new_value| { |
There was a problem hiding this comment.
I don't think the existing implementation have to change, or do they?
There was a problem hiding this comment.
Perhaps not, although conceptually an accumulator is just a map-reduce-map via some monoid or semigroup. The current implementation supports only monoids (need an empty value) but it doesn't support semigroups (no empty value). We could use this for something like FirstValue or AnyValue but I guess we could also implement it specifically for that use case.
There was a problem hiding this comment.
We could even drop the initial value parameter since it doesn't matter in this case.
joroKr21
commented
Jul 24, 2024
@alamb the benchmark didn't trigger on comment |
Dandandan
commented
Jul 24, 2024
joroKr21
commented
Jul 24, 2024
Sure, it might be some time until I do that though |
joroKr21
commented
Aug 27, 2024
I don't like this API at all |
Which issue does this PR close?
Closes#11591.
Rationale for this change
Provide more flexibility for implementing
GroupsAccumulatorwhich often make use ofNullState.What changes are included in this PR?
Add
was_validparameter toNullStatecallbacks. In this way implementations can handle nulls differently.Are these changes tested?
Yes, extended existing tests.
Are there any user-facing changes?
Yes, changes the signatures of
NullState::accumulateandNullState::accumulate_boolean.