Skip to content

GREATEST/LEAST post-aggregators in SQL#8719

Merged
jon-wei merged 20 commits intoapache:masterfrom
aditya-r-m:feat-greatestLeastSQLAggs
Feb 5, 2020
Merged

GREATEST/LEAST post-aggregators in SQL#8719
jon-wei merged 20 commits intoapache:masterfrom
aditya-r-m:feat-greatestLeastSQLAggs

Conversation

@aditya-r-m
Copy link
Contributor

@aditya-r-m aditya-r-m commented Oct 22, 2019 •
edited
Loading

Fixes #8679

Created & Registered SQL aggregation functions for native GREATEST/LEAST post aggregators

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths.
Key changed/added classes in this PR
  • MultiColumnSqlAggregator.java
  • GreatestSqlAggregator
  • LeastSqlAggregator
  • DruidOperatorTable

@lgtm-com
Copy link

lgtm-com bot commented Oct 25, 2019

This pull request introduces 2 alerts when merging b68805f into b9d1047 - view on LGTM.com

new alerts:

  • 2 for Boxed variable is never null

@aditya-r-m aditya-r-m marked this pull request as ready for review October 26, 2019 18:27
@aditya-r-m aditya-r-m changed the title [WIP] GREATEST/LEAST post-aggregators in SQL GREATEST/LEAST post-aggregators in SQL Oct 26, 2019
nishantmonu51
nishantmonu51 approved these changes Oct 31, 2019
View reviewed changes
Copy link
Member

@nishantmonu51 nishantmonu51 left a comment

Choose a reason for hiding this comment

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

👍 LGTM

suneet-s
suneet-s reviewed Dec 19, 2019
View reviewed changes
Copy link
Contributor

@suneet-s suneet-s left a comment

Choose a reason for hiding this comment

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

Thanks for the commit! Overall looks cool - I have a recommendation on how to re-structure some of this. My only real concern is with how we build the aggregator and post aggregators for floats.

I haven't reviewed the tests yet.

@suneet-s
Copy link
Contributor

suneet-s commented Dec 20, 2019

Thanks for the quick turn around @aditya-r-m. I'm not very familiar with the structure of Aggregators/ PostAggregators so I need to dig into that area of the code a little more.

I'll pick up this review again some time in Christmas week and get back to you soon.

@aditya-r-m
Copy link
Contributor Author

aditya-r-m commented Dec 20, 2019

@suneet-s thank you for the guidance & update.
I'd be available in case any update is required from my end.

@aditya-r-m aditya-r-m requested a review from gianm January 25, 2020 12:24
suneet-s
suneet-s reviewed Feb 2, 2020
View reviewed changes
Copy link
Contributor

@suneet-s suneet-s left a comment

Choose a reason for hiding this comment

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

@aditya-r-m Thanks for your patience with this. I had to learn a little how post aggregators worked. Thy work on aggregated values so this should be efficient.

2 potential issues

  1. using a deprecated constructor
  2. The ReturnType for the SqlAggFunctions

Bonus - more tests for maintainability :) Otherwise LGTM

@aditya-r-m
Copy link
Contributor Author

aditya-r-m commented Feb 4, 2020

@suneet-s thanks again for your guidance.
I have implemented all the suggested improvements, tests & additional documentation.

suneet-s
suneet-s approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@suneet-s suneet-s left a comment

Choose a reason for hiding this comment

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

Nice job! Thanks for the new feature! 💪 🎉

El gee tee emm

@jon-wei jon-wei merged commit 868fdeb into apache:master Feb 5, 2020
maytasm pushed a commit to maytasm/druid that referenced this pull request Feb 26, 2020
@aditya-r-m @maytasm
GREATEST/LEAST post-aggregators in SQL (apache#8719)
364523a 
* implement shell for greatest sql aggregator with hardcoded long values

* implement functional long greatest aggregator for direct access columns

* implement greatest & least sql aggregators for long & double types using abstract base class

* add javadocs, unit tests & handling for floats for greatest/least postaggregations

* minor checkstyle fix

* improve naming for the test cases

* make inner class static

* remove blank lines to retest travis build

* change trivial text to rerun travis build

* implement suggested updates for greatest/least sql aggs & fix checkstyle issues

* fix stale comments in greatest/least sql aggs abstract base

* Update sql.md

* improve sql function definitions for greatest/least sql aggs

* add more tests for greatest/least sql aggs

* add tests to cover invalid greatest/least sql expressions

* rename & reorder greatest least sql tests
maytasm added a commit to implydata/druid-public that referenced this pull request Feb 26, 2020
@maytasm
Merge pull request #63 from maytasm3/bp-great-least
ce62938 
[Backport] GREATEST/LEAST post-aggregators in SQL (apache#8719)
ccaominh added a commit to implydata/druid-public that referenced this pull request Feb 29, 2020
@ccaominh
Revert "GREATEST/LEAST post-aggregators in SQL (apache#8719)"
7304db3 
This reverts commit 364523a.
ccaominh added a commit to implydata/druid-public that referenced this pull request Feb 29, 2020
@ccaominh
Revert "GREATEST/LEAST post-aggregators in SQL (apache#8719)"
039284b 
This reverts commit 364523a.
@ccaominh ccaominh mentioned this pull request Mar 9, 2020
Merged
5 tasks
@jihoonson jihoonson added this to the 0.18.0 milestone Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nishantmonu51 nishantmonu51 nishantmonu51 approved these changes

@jon-wei jon-wei jon-wei approved these changes

@suneet-s suneet-s suneet-s approved these changes

@gianm gianm Awaiting requested review from gianm

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.18.0

Development

Successfully merging this pull request may close these issues.

Feature request to support GREATEST/LEAST function in Druid sql

5 participants

@aditya-r-m @suneet-s @nishantmonu51 @jon-wei @jihoonson