Repository navigation
Refactor bit aggregate functions signature - #18593
Conversation
| signature: Signature::uniform(1, INTEGERS.to_vec(), Volatility::Immutable), | ||
| signature: Signature::coercible( | ||
| vec![Coercion::new_exact(TypeSignatureClass::Integer)], | ||
| Volatility::Immutable, | ||
| ), |
There was a problem hiding this comment.
Main change here; a consequence is DataType::Null input comes in as DataType::Null instead of previously being casted to DataType::Int8.
| macro_rules! downcast_bitwise_accumulator { | ||
| ($args:ident, $opr:expr, $is_distinct: expr) => { | ||
| match $args.return_field.data_type() { | ||
| DataType::Null => Ok(Box::new(NoopAccumulator::default())), |
There was a problem hiding this comment.
Handling of Null input type; introduce a new NoopAccumulator which can be used for other aggregate functions, like approx_distinct
| default_accumulator_impl!(); | ||
| } | ||
|
|
||
| impl Accumulator for NullHLLAccumulator { |
There was a problem hiding this comment.
Technically unrelated to the main change, but since I introduce NoopAccumulator I figured it would be good to use it in other places too, so refactored this.
| /// [`Accumulator`] that does no work and always returns a fixed value (default | ||
| /// of `NULL` but can be customized). | ||
| /// | ||
| /// Useful for aggregate functions that need to handle an input of [`DataType::Null`] | ||
| /// that does no work. | ||
| /// | ||
| /// [`DataType::Null`]: arrow::datatypes::DataType::Null | ||
| #[derive(Debug)] | ||
| pub struct NoopAccumulator { | ||
| evaluate_value: ScalarValue, | ||
| } |
There was a problem hiding this comment.
I looked into other ideas like trying to implement simplify on AggregateUDFImpl to return Null directly instead of having this noop accumulator but couldn't get it to work; figured can at least centralize this boilerplate and look into easier/more ergonomic handling of pure null inputs for UDFs later
|
|
||
| fn state_fields(&self, args: StateFieldsArgs) -> Result<Vec<FieldRef>> { | ||
| if self.operation == BitwiseOperationType::Xor && args.is_distinct { | ||
| if args.input_fields[0].data_type().is_null() { |
There was a problem hiding this comment.
Needing to change state_fields is not desirable, but until we can make accumulators own state_fields (instead of the function) this will have to do
| query I??III?T | ||
| query I??????T | ||
| select count(null), min(null), max(null), bit_and(NULL), bit_or(NULL), bit_xor(NULL), nth_value(NULL, 1), string_agg(NULL, ','); | ||
| ---- | ||
| 0 NULL NULL NULL NULL NULL NULL NULL | ||
|
|
||
| query TTT | ||
| SELECT arrow_typeof(bit_and(NULL)), arrow_typeof(bit_or(NULL)), arrow_typeof(bit_xor(NULL)) | ||
| ---- | ||
| Null Null Null |
There was a problem hiding this comment.
Note how null inputs now have null output types too
Which issue does this PR close?
Part of #18092
Rationale for this change
Use simpler coercion API.
What changes are included in this PR?
Use coercion API for bit aggregate functions(
bit_and,bit_or,bit_xor), instead of enumerating numeric types.Handle
Nulldatatype explicitly in above functions.Introduce
NoopAccumulatorfor easier handling of the above.Are these changes tested?
Existing tests.
Are there any user-facing changes?
No.