jayzhan211 commented on code in PR #7629:
URL: https://github.com/apache/arrow-datafusion/pull/7629#discussion_r1357637090
##########
datafusion/physical-expr/src/aggregate/array_agg_ordered.rs:
##########
@@ -280,41 +283,44 @@ impl Accumulator for OrderSensitiveArrayAggAccumulator {
}
impl OrderSensitiveArrayAggAccumulator {
+ /// Inner Vec\<ScalarValue> in the ordering_values can be thought as
ordering information for the each ScalarValue in the values array.
+ /// See [`merge_ordered_arrays`] for more information.
fn convert_array_agg_to_orderings(
&self,
- in_data: ScalarValue,
- ) -> Result<Vec<Vec<ScalarValue>>> {
- if let ScalarValue::List(Some(list_vals), _field_ref) = in_data {
- list_vals.into_iter().map(|struct_vals| {
- if let ScalarValue::Struct(Some(orderings), _fields) =
struct_vals {
- Ok(orderings)
- } else {
- exec_err!(
- "Expects to receive ScalarValue::Struct(Some(..), _)
but got:{:?}",
- struct_vals.data_type()
- )
- }
- }).collect::<Result<Vec<_>>>()
- } else {
- exec_err!(
- "Expects to receive ScalarValue::List(Some(..), _) but
got:{:?}",
- in_data.data_type()
- )
+ array_agg: Vec<Vec<ScalarValue>>,
+ ) -> Result<Vec<Vec<Vec<ScalarValue>>>> {
+ let mut orderings = vec![];
+ // in_data is Vec<ScalarValue> where ScalarValue does not include
ScalarValue::List
+ for in_data in array_agg.into_iter() {
+ let ordering = in_data.into_iter().map(|struct_vals| {
+ if let ScalarValue::Struct(Some(orderings), _) =
struct_vals {
Review Comment:
The reason we have 3 levels of Vec in `orderings` is because we got
`Vec\<ScalarValue>` in Struct, but the value here seems to have only one
`orderings.len() == 1` based on the test.
Is it reasonable to have Vec of orderings here?
--
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]
