-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: add array_min scalar function and associated tests
#16574
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
Conversation
alamb
left a comment
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.
Thank you @dharanad -- this looks very nice.
I suspect this function could be substantially sped up with a specialized implementation but we can defer that to when someone is interested in doing so
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| use crate::utils::make_scalar_function; |
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 recommend putting this in the same file as https://github.com/apache/datafusion/blob/main/datafusion/functions-nested/src/max.rs
perhaps call it https://github.com/apache/datafusion/blob/main/datafusion/functions-nested/src/min_max.rs 🤔
You may be able to share some code, but more importantly I think it will be clearer that min and max are basically the same function
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.
Sure, should be quick. Let me move refac
"specialized implementation" sounds interesting, can you please ellaborate more i can try working on it on a sperate issue ? |
… better code reuse
Well the first thing would be to make a benchmark calling array_min for different types of ListArrays (like LIst and List) with different length list elements The profile with these instructions https://datafusion.apache.org/library-user-guide/profiling.html#profile-the-benchmark My guess (needs to be verified by profiling):
|
|
Thanks for the details @alamb I will try this out and share my findings |
|
Thanks again @dharanad |
Which issue does this PR close?
array_minscalar function #16570Rationale for this change
What changes are included in this PR?
Implemented ArrayMin Scalar function
Are these changes tested?
Yes, Add logic tests as well
Are there any user-facing changes?