-
Notifications
You must be signed in to change notification settings - Fork 24.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ES|QL] Add mv_sort #106095
[ES|QL] Add mv_sort #106095
Conversation
Documentation preview: |
Pinging @elastic/es-analytical-engine (Team:Analytics) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments around testing and moving the order
resolution. And maybe renaming a method. But it looks good!
...pute/src/main/generated-src/org/elasticsearch/compute/operator/MultivalueDedupeBytesRef.java
Outdated
Show resolved
Hide resolved
...pute/src/main/generated-src/org/elasticsearch/compute/operator/MultivalueDedupeBytesRef.java
Outdated
Show resolved
Hide resolved
...pute/src/main/generated-src/org/elasticsearch/compute/operator/MultivalueDedupeBytesRef.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSort.java
Outdated
Show resolved
Hide resolved
...est/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSortTests.java
Outdated
Show resolved
Hide resolved
...est/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSortTests.java
Show resolved
Hide resolved
Thanks for reviewing, the input to sortToBlock that indicates the sort order is converted to a boolean, if there is a better way to do it, any suggestion is welcome! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a small thing about the stringtemplate stuff and medium sized thing around the boolean translation. I think there's a better way to do it. The way you are doing it I think is fairly confusing. I had a to read it a few times and I think it's possible it'll do unexpected things.
...gin/esql/compute/src/main/java/org/elasticsearch/compute/operator/X-MultivalueDedupe.java.st
Outdated
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSort.java
Outdated
Show resolved
Hide resolved
A subtask of #105322 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left a couple of nice to haves. It's ready whenever you are ready to merge it and you can do any or none of the nice-to-haves.
...gin/esql/compute/src/main/java/org/elasticsearch/compute/operator/X-MultivalueDedupe.java.st
Show resolved
Hide resolved
...n/esql/compute/src/main/java/org/elasticsearch/compute/operator/MultivalueDedupeBoolean.java
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSort.java
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSort.java
Show resolved
Hide resolved
...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSort.java
Outdated
Show resolved
Hide resolved
...est/java/org/elasticsearch/xpack/esql/expression/function/scalar/multivalue/MvSortTests.java
Show resolved
Hide resolved
good luck with all the merges. |
Thanks for reviewing Nik! All of the comments are addressed. |
mv_sort sorts a multivalued field in ascending or descending order. If an order is not specified, the default is in ascending order.