You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jayzhan211 (via GitHub)" <gi...@apache.org> on 2024/02/15 02:26:51 UTC
[PR] Support List for Array aggregate order [arrow-datafusion]
jayzhan211 opened a new pull request, #9234:
URL: https://github.com/apache/arrow-datafusion/pull/9234
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #8512.
There are value expressions and ordering expressions in array aggregate order, this PR supports List type for value expression.
List type for Ordering expressions is not included.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
I came out with another test example with the same error, I didn't directly test the example given from the issue.
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support List for Array aggregate order and distinct [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9234:
URL: https://github.com/apache/arrow-datafusion/pull/9234#discussion_r1492366835
##########
datafusion/common/src/scalar/mod.rs:
##########
@@ -2143,23 +2142,50 @@ impl ScalarValue {
/// use datafusion_common::ScalarValue;
/// use arrow::array::ListArray;
/// use arrow::datatypes::{DataType, Int32Type};
+ /// use datafusion_common::utils::array_into_list_array;
+ /// use std::sync::Arc;
///
/// let list_arr = ListArray::from_iter_primitive::<Int32Type, _, _>(vec![
/// Some(vec![Some(1), Some(2), Some(3)]),
- /// None,
/// Some(vec![Some(4), Some(5)])
/// ]);
///
/// let scalar_vec = ScalarValue::convert_array_to_scalar_vec(&list_arr).unwrap();
///
/// let expected = vec![
- /// vec![
+ /// vec![
/// ScalarValue::Int32(Some(1)),
/// ScalarValue::Int32(Some(2)),
/// ScalarValue::Int32(Some(3)),
+ /// ],
+ /// vec![
+ /// ScalarValue::Int32(Some(4)),
+ /// ScalarValue::Int32(Some(5)),
+ /// ],
+ /// ];
+ ///
+ /// assert_eq!(scalar_vec, expected);
Review Comment:
Thank you fro these comments -- I suggest we improve the docs even more by:
1. Split the examples up (in this case add a second `# Example of using array_into_list_array` heading and a second doc example
2. Add some comments explaining what each line is doing. For you it is likely obvious, but for a causal reader,. understanding what `array_into_list_array` is doing or why this is different from the previous example is likely not as easy
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -139,6 +139,61 @@ AggregateExec: mode=Final, gby=[], aggr=[ARRAY_AGG(agg_order.c1)]
--------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1
----------CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/aggregate_agg_multi_order.csv]]}, projection=[c1, c2, c3], has_header=true
+# test array_agg_order with list data type
+statement ok
+CREATE TABLE array_agg_order_list_table AS VALUES
+ ('w', 2, [1,2,3], 10),
+ ('w', 1, [9,5,2], 20),
+ ('w', 1, [3,2,5], 30),
+ ('b', 2, [4,5,6], 20),
+ ('b', 1, [7,8,9], 30)
+;
+
+query T? rowsort
+select column1, array_agg(column3 order by column2, column4 desc) from array_agg_order_list_table group by column1;
+----
+b [[7, 8, 9], [4, 5, 6]]
+w [[3, 2, 5], [9, 5, 2], [1, 2, 3]]
+
+query T?? rowsort
+select column1, first_value(column3 order by column2, column4 desc), last_value(column3 order by column2, column4 desc) from array_agg_order_list_table group by column1;
+----
+b [7, 8, 9] [4, 5, 6]
+w [3, 2, 5] [1, 2, 3]
+
+query T? rowsort
+select column1, nth_value(column3, 2 order by column2, column4 desc) from array_agg_order_list_table group by column1;
+----
+b [4, 5, 6]
+w [9, 5, 2]
+
+statement ok
+drop table array_agg_order_list_table;
+
+# test array_agg_distinct with list data type
+statement ok
+CREATE TABLE array_agg_distinct_list_table AS VALUES
+ ('w', [0,1]),
+ ('w', [0,1]),
+ ('w', [1,0]),
+ ('b', [1,0]),
+ ('b', [1,0]),
+ ('b', [1,0]),
+ ('b', [0,1])
+;
+
+# Apply array_sort to have determinisitic result, higher dimension nested array also works but not for array sort,
+# so they are covered in `datafusion/physical-expr/src/aggregate/array_agg_distinct.rs`
+query ??
+select array_sort(c1), array_sort(c2) from (
Review Comment:
Yeah, we would need to use a structure for deduplicating that preserved input order
##########
datafusion/physical-expr/src/aggregate/array_agg_distinct.rs:
##########
@@ -151,7 +153,11 @@ impl Accumulator for DistinctArrayAggAccumulator {
return Ok(());
}
- self.update_batch(states)
+ let array = &states[0];
+
+ // Unwrap outer ListArray then do update batch
+ let inner_array = array.as_list::<i32>().value(0);
Review Comment:
How do we know there is only a single row in the states array? Can we please add an assert here to make sure we don't accidentally lose data?
Something like `assert_eq!(array.len(), 1)`?
##########
datafusion/physical-expr/src/aggregate/array_agg_distinct.rs:
##########
@@ -181,47 +187,55 @@ mod tests {
use arrow_array::Array;
use arrow_array::ListArray;
use arrow_buffer::OffsetBuffer;
- use datafusion_common::utils::array_into_list_array;
use datafusion_common::{internal_err, DataFusionError};
- // arrow::compute::sort cann't sort ListArray directly, so we need to sort the inner primitive array and wrap it back into ListArray.
- fn sort_list_inner(arr: ScalarValue) -> ScalarValue {
- let arr = match arr {
- ScalarValue::List(arr) => arr.value(0),
- _ => {
- panic!("Expected ScalarValue::List, got {:?}", arr)
- }
- };
+ // arrow::compute::sort can't sort nested ListArray directly, so we compare the scalar values pair-wise.
Review Comment:
What do you think about filing a ticket upstream in arrow-rs to properly support sorting nested lists?
It seems like it would be valuable eventually and could likely be done much more efficiently than using `ScalarValue`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support List for Array aggregate order [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9234:
URL: https://github.com/apache/arrow-datafusion/pull/9234#discussion_r1490554114
##########
datafusion/physical-expr/src/aggregate/array_agg_distinct.rs:
##########
@@ -351,23 +358,16 @@ mod tests {
let l2 = ScalarValue::List(Arc::new(l2));
let l3 = ScalarValue::List(Arc::new(l3));
- // Duplicate l1 in the input array and check that it is deduped in the output.
- let array = ScalarValue::iter_to_array(vec![l1.clone(), l2, l3, l1]).unwrap();
-
- let expected =
- ScalarValue::List(Arc::new(
- ListArray::from_iter_primitive::<Int32Type, _, _>(vec![Some(vec![
- Some(1),
- Some(2),
- Some(3),
- Some(4),
- Some(5),
- Some(6),
- Some(7),
- Some(8),
- Some(9),
- ])]),
- ));
+ // Duplicate l1 and l3 in the input array and check that it is deduped in the output.
Review Comment:
This seems more correct to me. The previous flattened result is incorrect.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support List for Array aggregate order and distinct [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9234:
URL: https://github.com/apache/arrow-datafusion/pull/9234#discussion_r1490598409
##########
datafusion/common/src/scalar.rs:
##########
@@ -2084,27 +2110,13 @@ impl ScalarValue {
let mut scalars = Vec::with_capacity(array.len());
for index in 0..array.len() {
- let scalar_values = match array.data_type() {
- DataType::List(_) => {
- let list_array = as_list_array(array);
- match list_array.is_null(index) {
- true => Vec::new(),
- false => {
- let nested_array = list_array.value(index);
- ScalarValue::convert_array_to_scalar_vec(&nested_array)?
- .into_iter()
- .flatten()
- .collect()
- }
- }
- }
- _ => {
- let scalar = ScalarValue::try_from_array(array, index)?;
- vec![scalar]
- }
- };
+ let nested_array = array.as_list::<i32>().value(index);
Review Comment:
Main fix
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support List for Array aggregate order and distinct [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9234:
URL: https://github.com/apache/arrow-datafusion/pull/9234#issuecomment-1956064959
🎉
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support List for Array aggregate order and distinct [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #9234:
URL: https://github.com/apache/arrow-datafusion/pull/9234#issuecomment-1948222371
This is ready for review. Only rebase if needed
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support List for Array aggregate order [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9234:
URL: https://github.com/apache/arrow-datafusion/pull/9234#discussion_r1490555342
##########
datafusion/common/src/scalar.rs:
##########
@@ -2108,6 +2108,57 @@ impl ScalarValue {
Ok(scalars)
}
+ /// convert_array_to_scalar_vec but only convert the first level instead of recursively converting
+ /// all the levels, so list remains as ScalarValue::List
+ ///
+ /// Example
+ /// ```
+ /// use datafusion_common::ScalarValue;
+ /// use arrow::array::ListArray;
+ /// use arrow::datatypes::{DataType, Int32Type};
+ /// use datafusion_common::utils::array_into_list_array;
+ /// use std::sync::Arc;
+ ///
+ /// let list_arr = ListArray::from_iter_primitive::<Int32Type, _, _>(vec![
+ /// Some(vec![Some(1), Some(2), Some(3)]),
+ /// Some(vec![Some(4), Some(5)])
+ /// ]);
+ /// let list_arr = array_into_list_array(Arc::new(list_arr));
+ ///
+ /// let scalar_vec = ScalarValue::convert_first_level_array_to_scalar_vec(&list_arr).unwrap();
+ ///
+ /// let l1 = ListArray::from_iter_primitive::<Int32Type, _, _>(vec![
+ /// Some(vec![Some(1), Some(2), Some(3)]),
+ /// ]);
+ /// let l2 = ListArray::from_iter_primitive::<Int32Type, _, _>(vec![
+ /// Some(vec![Some(4), Some(5)]),
+ /// ]);
+ ///
+ /// let expected = vec![
+ /// vec![
+ /// ScalarValue::List(Arc::new(l1)),
+ /// ScalarValue::List(Arc::new(l2)),
+ /// ],
+ /// ];
+ ///
+ /// assert_eq!(scalar_vec, expected);
+ /// ```
+ pub fn convert_first_level_array_to_scalar_vec(
Review Comment:
I think we may not need the current version of `convert_array_to_scalar_vec`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support List for Array aggregate order and distinct [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9234:
URL: https://github.com/apache/arrow-datafusion/pull/9234#discussion_r1490598409
##########
datafusion/common/src/scalar.rs:
##########
@@ -2084,27 +2110,13 @@ impl ScalarValue {
let mut scalars = Vec::with_capacity(array.len());
for index in 0..array.len() {
- let scalar_values = match array.data_type() {
- DataType::List(_) => {
- let list_array = as_list_array(array);
- match list_array.is_null(index) {
- true => Vec::new(),
- false => {
- let nested_array = list_array.value(index);
- ScalarValue::convert_array_to_scalar_vec(&nested_array)?
- .into_iter()
- .flatten()
- .collect()
- }
- }
- }
- _ => {
- let scalar = ScalarValue::try_from_array(array, index)?;
- vec![scalar]
- }
- };
+ let nested_array = array.as_list::<i32>().value(index);
Review Comment:
The main fix is to avoid recursively flattening, just convert each row directly.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support List for Array aggregate order and distinct [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #9234:
URL: https://github.com/apache/arrow-datafusion/pull/9234
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support List for Array aggregate order and distinct [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9234:
URL: https://github.com/apache/arrow-datafusion/pull/9234#discussion_r1492614422
##########
datafusion/physical-expr/src/aggregate/array_agg_distinct.rs:
##########
@@ -181,47 +187,55 @@ mod tests {
use arrow_array::Array;
use arrow_array::ListArray;
use arrow_buffer::OffsetBuffer;
- use datafusion_common::utils::array_into_list_array;
use datafusion_common::{internal_err, DataFusionError};
- // arrow::compute::sort cann't sort ListArray directly, so we need to sort the inner primitive array and wrap it back into ListArray.
- fn sort_list_inner(arr: ScalarValue) -> ScalarValue {
- let arr = match arr {
- ScalarValue::List(arr) => arr.value(0),
- _ => {
- panic!("Expected ScalarValue::List, got {:?}", arr)
- }
- };
+ // arrow::compute::sort can't sort nested ListArray directly, so we compare the scalar values pair-wise.
Review Comment:
> Agree, let me file the ticket. But, I think it is not so helpful for this test (values are little). Maybe support it if there is somewhere that needs sorting many nested lists.
Actually, we can just support `array_sort` for nested array, this also need upstream fix
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support List for Array aggregate order [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9234:
URL: https://github.com/apache/arrow-datafusion/pull/9234#discussion_r1490557328
##########
datafusion/sqllogictest/test_files/aggregate.slt:
##########
@@ -139,6 +139,61 @@ AggregateExec: mode=Final, gby=[], aggr=[ARRAY_AGG(agg_order.c1)]
--------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1
----------CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/aggregate_agg_multi_order.csv]]}, projection=[c1, c2, c3], has_header=true
+# test array_agg_order with list data type
+statement ok
+CREATE TABLE array_agg_order_list_table AS VALUES
+ ('w', 2, [1,2,3], 10),
+ ('w', 1, [9,5,2], 20),
+ ('w', 1, [3,2,5], 30),
+ ('b', 2, [4,5,6], 20),
+ ('b', 1, [7,8,9], 30)
+;
+
+query T? rowsort
+select column1, array_agg(column3 order by column2, column4 desc) from array_agg_order_list_table group by column1;
+----
+b [[7, 8, 9], [4, 5, 6]]
+w [[3, 2, 5], [9, 5, 2], [1, 2, 3]]
+
+query T?? rowsort
+select column1, first_value(column3 order by column2, column4 desc), last_value(column3 order by column2, column4 desc) from array_agg_order_list_table group by column1;
+----
+b [7, 8, 9] [4, 5, 6]
+w [3, 2, 5] [1, 2, 3]
+
+query T? rowsort
+select column1, nth_value(column3, 2 order by column2, column4 desc) from array_agg_order_list_table group by column1;
+----
+b [4, 5, 6]
+w [9, 5, 2]
+
+statement ok
+drop table array_agg_order_list_table;
+
+# test array_agg_distinct with list data type
+statement ok
+CREATE TABLE array_agg_distinct_list_table AS VALUES
+ ('w', [0,1]),
+ ('w', [0,1]),
+ ('w', [1,0]),
+ ('b', [1,0]),
+ ('b', [1,0]),
+ ('b', [1,0]),
+ ('b', [0,1])
+;
+
+# Apply array_sort to have determinisitic result, higher dimension nested array also works but not for array sort,
+# so they are covered in `datafusion/physical-expr/src/aggregate/array_agg_distinct.rs`
+query ??
+select array_sort(c1), array_sort(c2) from (
Review Comment:
Rather than sorting, if there is some setting or random seed that comes out the deterministic result, it would be great
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support List for Array aggregate order and distinct [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9234:
URL: https://github.com/apache/arrow-datafusion/pull/9234#discussion_r1492604001
##########
datafusion/physical-expr/src/aggregate/array_agg_distinct.rs:
##########
@@ -181,47 +187,55 @@ mod tests {
use arrow_array::Array;
use arrow_array::ListArray;
use arrow_buffer::OffsetBuffer;
- use datafusion_common::utils::array_into_list_array;
use datafusion_common::{internal_err, DataFusionError};
- // arrow::compute::sort cann't sort ListArray directly, so we need to sort the inner primitive array and wrap it back into ListArray.
- fn sort_list_inner(arr: ScalarValue) -> ScalarValue {
- let arr = match arr {
- ScalarValue::List(arr) => arr.value(0),
- _ => {
- panic!("Expected ScalarValue::List, got {:?}", arr)
- }
- };
+ // arrow::compute::sort can't sort nested ListArray directly, so we compare the scalar values pair-wise.
Review Comment:
Agree, let me file the ticket. But, I think it is not so helpful for this test (values are little). Maybe support it if there is somewhere that needs sorting many nested lists.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org