Skip to content
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

feat: Support MIN/MAX udafs for Time/TS/Date types #8924

Merged
merged 1 commit into from
Mar 30, 2022

Conversation

pgaref
Copy link
Member

@pgaref pgaref commented Mar 24, 2022

Description

Natively support MIN/MAX aggregations for DATE, TIME, and TIMESTAMP types.
All existing types implement Comparable interface so a base BaseComparableKudaf replaced BaseNumberKudaf and appropriate 2 Min/Max Kudafs were implemented (getting rid of 12+ type specific implementations).

Addresses #8757

Testing done

Added unit Tests for every type - Min/Max combination
Extended max-group-by min-group-by functional tests

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@pgaref pgaref requested a review from a team as a code owner March 24, 2022 03:41
@jnh5y
Copy link
Member

jnh5y commented Mar 24, 2022

Also, you'll want to add the new types to AggregateFunctionFactory.java line 44 or so. That's used by the DESCRIBE FUNCTION command.

@jnh5y
Copy link
Member

jnh5y commented Mar 24, 2022

Also, you'll want to add the new types to AggregateFunctionFactory.java line 44 or so. That's used by the DESCRIBE FUNCTION command.

Actually, I misspoke, you'll want to update supportedArgs() for each of the Min|MaxAggFunctionFactory classes. ;)

@pgaref pgaref force-pushed the min-max-date-ts branch 2 times, most recently from d0ce164 to 32c1173 Compare March 25, 2022 02:04
Copy link
Member

@spena spena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pgaref pgaref changed the title feat: Support MIN/MAX udafs for Time/TS/Date types (#8757) feat: Support MIN/MAX udafs for Time/TS/Date types Mar 30, 2022
@pgaref pgaref merged commit 6399d30 into confluentinc:master Mar 30, 2022
@pgaref pgaref deleted the min-max-date-ts branch March 30, 2022 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants