kosiew commented on code in PR #21315:
URL: https://github.com/apache/datafusion/pull/21315#discussion_r3116030962
##########
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:
Thanks for pointing out the inconsistency in the current framing.
I think the safest way to resolve it is not to broaden the “logically
equivalent values” story, but to narrow the documented contract back to the
cases the accumulator actually expects. In normal min/max aggregation, both
dictionary scalars should come from the same aggregate input type, so differing
dictionary key types are not a supported user-facing scenario so much as an
unexpected invariant break.
So I would avoid presenting this as general mixed-type comparison support.
Instead, I’d document that dictionary unwrapping exists to preserve logical
value comparison for the expected accumulator flow, and I’d keep mismatched
dictionary key types explicit as an internal-error case unless we intentionally
decide to support cross-key-type dictionaries more broadly.
--
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]
