feat: Support array_sort(list_sort)
#8279
Conversation
# Conflicts: # datafusion/physical-plan/src/joins/hash_join_utils.rs
# Conflicts: # datafusion/physical-plan/src/joins/hash_join_utils.rs
# Conflicts: # datafusion/proto/src/generated/prost.rs # datafusion/proto/src/logical_plan/from_proto.rs
github-actions
bot
added
logical-expr
There was a problem hiding this comment.
Since DuckDB has array_sort already, and I think there is no good reason that we should not follow theirs. Can you have the similar style like theirs?
https://duckdb.org/docs/sql/functions/nested#sorting-lists
Especially, I think array_sort(list, 'asc') or array_sort(list, 'desc') is more much straightforward then true, false. We need to check one of the result to know whether true is asc or desc. So as null_first
| @@ -2669,6 +2698,48 @@ mod tests { | |||
| ); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_array_sort() { | |||
There was a problem hiding this comment.
Dont add test here, just add them in slt file
I'll try |
| @@ -1044,6 +1044,20 @@ select make_array(['a','b'], null); | |||
| ---- | |||
| [[a, b], ] | |||
|
|
|||
| ## array_sort (aliases: `list_sort`) | |||
| query ??? | |||
| select array_sort(make_array(4, 2, 3, 1)), array_sort(make_array(1.0, 4.0, null, 3.0), true, true), array_sort(make_array('a', 'd', 'c', null, 'b'), false, false); | |||
There was a problem hiding this comment.
btw, you can also try column-wise support, pass the test with table, you can refer other array_function that run with table. But it is fine for me to support that in follow on PR
| [, 11, 12, 13, 14, 15, 16, 17, 18, 20] | ||
| [, 21, 22, 23, 25, 26, 27, 28, 29, 30] | ||
| [, 31, 32, 33, 34, 35, 37, 38, 39, 40] | ||
| [] |
There was a problem hiding this comment.
array_sort(null) should return null, not []
There was a problem hiding this comment.
# Conflicts: # datafusion/proto/proto/datafusion.proto # datafusion/proto/src/generated/pbjson.rs # datafusion/proto/src/generated/prost.rs
|
cc @alamb |
There was a problem hiding this comment.
Thank you @Asura7969 (and @jayzhan211 and @Weijun-H for the reviews), and I am really sorry for taking so long to look at this PR
I think all that is needed now is to merge up from main, and resolve some conflicts, and this PR will be ready to merge
Thanks again
# Conflicts: # datafusion/expr/src/built_in_function.rs # datafusion/proto/proto/datafusion.proto # datafusion/proto/src/generated/pbjson.rs # datafusion/proto/src/generated/prost.rs
|
Thanks again 🚀 |
* Minor: Improve the document format of JoinHashMap * list sort * fix: example doc * fix: ci * fix: doc error * fix pb * like DuckDB function semantics * fix ci * fix pb * fix: doc * add table test * fix: not as expected * fix: return null * resolve conflicts * doc * merge
Which issue does this PR close?
Closes #109 .
Rationale for this change
Support append_sort（list_sort） function
What changes are included in this PR?
Four array functions now start to support column data:
array_sortlist_sortAre these changes tested?
Yes
test_array_sortAre there any user-facing changes?
Yes