kosiew commented on code in PR #21315:
URL: https://github.com/apache/datafusion/pull/21315#discussion_r3135392744
##########
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:
hi @Jefffrey
I have reverted the changes to `min_max_batch_generic`.
--
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]
