kosiew commented on code in PR #21315:
URL: https://github.com/apache/datafusion/pull/21315#discussion_r3099691607
##########
datafusion/functions-aggregate-common/src/min_max.rs:
##########
@@ -413,6 +413,22 @@ macro_rules! min_max {
min_max_generic!(lhs, rhs, $OP)
}
+ (lhs, rhs)
+ if matches!(lhs, ScalarValue::Dictionary(_, _))
+ || matches!(rhs, ScalarValue::Dictionary(_, _)) =>
+ {
+ let (lhs, lhs_key_type) = dictionary_scalar_parts(lhs);
+ let (rhs, rhs_key_type) = dictionary_scalar_parts(rhs);
+ let result = min_max_generic!(lhs, rhs, $OP);
+
+ match lhs_key_type.zip(rhs_key_type) {
+ Some((key_type, _)) => {
+ ScalarValue::Dictionary(Box::new(key_type.clone()),
Box::new(result))
Review Comment:
In the intended accumulator flow there should not be any significance,
because both sides should already have the same dictionary datatype: the
accumulator state is created from the aggregate input type, and each batch
result should come back with that same dictionary key type. In that happy path,
preserving either side’s key type produces the same result.
But, the current code makes that assumption too implicitly.
Right now we unwrap both dictionary scalars, compare the inner values, and
then re-wrap using the left key type whenever both sides are dictionaries. If
the key types ever diverged unexpectedly, this would silently ignore the
right-hand key type instead of surfacing an error. I think the safer behavior
is to validate that both key types match and return an error if they do not,
then re-wrap using the validated key type.
--
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]
