kosiew commented on code in PR #21315:
URL: https://github.com/apache/datafusion/pull/21315#discussion_r3099657781
##########
datafusion/functions-aggregate-common/src/min_max.rs:
##########
@@ -423,6 +439,53 @@ macro_rules! min_max {
}};
}
+fn scalar_batch_extreme(values: &ArrayRef, ordering: Ordering) ->
Result<ScalarValue> {
Review Comment:
The refactor was mainly to fix dictionary semantics, not just to rename the
helper. The old `min_max_batch_generic` logic was equivalent for
`Struct`/`List`-like types, but it was not equivalent for `Dictionary`: the old
`DataType::Dictionary(_, _) => max_batch(values.as_any_dictionary().values())`
/ `min_batch(...)` path scanned the dictionary value buffer rather than the
logical rows. That means it could consider unreferenced dictionary values and
ignore null key positions, which is exactly what the new dictionary tests are
guarding against.
`scalar_batch_extreme` uses `ScalarValue::try_from_array(values, index)` so
it compares the logical row values instead of the backing dictionary values
array. However, I agree the current naming can be improved. I'll rename it to
`scalar_row_extreme` and add a short doc comment explaining why dictionaries
need this path.
--
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]
