Jefffrey commented on code in PR #21315:
URL: https://github.com/apache/datafusion/pull/21315#discussion_r3117728579
##########
datafusion/functions-aggregate-common/src/min_max.rs:
##########
@@ -760,37 +813,40 @@ pub fn min_batch(values: &ArrayRef) ->
Result<ScalarValue> {
min_binary_view
)
}
- DataType::Struct(_) => min_max_batch_generic(values,
Ordering::Greater)?,
- DataType::List(_) => min_max_batch_generic(values, Ordering::Greater)?,
- DataType::LargeList(_) => min_max_batch_generic(values,
Ordering::Greater)?,
- DataType::FixedSizeList(_, _) => {
- min_max_batch_generic(values, Ordering::Greater)?
- }
- DataType::Dictionary(_, _) => {
- let values = values.as_any_dictionary().values();
- min_batch(values)?
- }
+ DataType::Struct(_)
+ | DataType::List(_)
+ | DataType::LargeList(_)
+ | DataType::FixedSizeList(_, _)
+ | DataType::Dictionary(_, _) => min_max_batch_generic(values,
Ordering::Greater)?,
_ => min_max_batch!(values, min),
})
}
-/// Generic min/max implementation for complex types
-fn min_max_batch_generic(array: &ArrayRef, ordering: Ordering) ->
Result<ScalarValue> {
- if array.len() == array.null_count() {
- return ScalarValue::try_from(array.data_type());
- }
- let mut extreme = ScalarValue::try_from_array(array, 0)?;
- for i in 1..array.len() {
- let current = ScalarValue::try_from_array(array, i)?;
- if current.is_null() {
- continue;
+/// Finds the min/max by scanning logical rows via
`ScalarValue::try_from_array`.
+///
+/// This path is required for dictionary arrays because comparing
+/// `dictionary.values()` is not semantically correct: it can include
+/// unreferenced values and ignore null key positions.
+fn min_max_batch_generic(values: &ArrayRef, ordering: Ordering) ->
Result<ScalarValue> {
+ let mut index = 0;
Review Comment:
I still do not understand why this function is being changed. The provided
reasoning makes no sense considering the original version already works as
intended. Especially considering it is not the responsibility of this function
that `dictionary.values()` isn't semantically correct; it is the responsibility
of the caller, which is not being fixed by refactoring this function. Can we
please revert the changes to this function.
--
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]
