-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Implement min/max for interval types #11015
Conversation
Thanks @maxburke for taking this! Could you also implement for the remaining two interval types? Except |
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 @maxburke and @waynexia
I also think this PR needs some tests; I wrote some for your consideration here urbanlogiq#91 (which also cover the three interval types)
Marking as draft while feedback is addressed (I a trying to work through the review queue) |
aa490eb
to
a879c54
Compare
@alamb merged; and feedback addressed |
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 @maxburke -- this looks great 🙏
It just needs an expected output update to get the CI to pass and I think it will be good to go (I left a note on how to do so)
Thank you again for the contribution
# aggregate_decimal_min | ||
query RT | ||
select min(c1), arrow_typeof(min(c1)) from d_table | ||
# aggregate Interval(MonthDayNano) min/max |
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 think these tests needs to be updated now that you have implemented the correct logic
You can do so by running
cargo test --test sqllogictests -- aggregate --complete
And then verifying and checking in the results
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.
updated; I think they look right at a glance, but I'm not well versed in this part of the system. can you verify?
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.
They looked good to me -- thanks!
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.
Looks good to me -- thanks again @maxburke
* Implement min/max for interval types * Add sqllogictests for min/max intervals * Add tests for interval min/max * update sql logic tests --------- Co-authored-by: Andrew Lamb <[email protected]>
Closes #11014