Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Support aggregations min, max #541

Merged
merged 18 commits into from Sep 30, 2020
Merged

Support aggregations min, max #541

merged 18 commits into from Sep 30, 2020

Conversation

chloe-zh
Copy link
Member

Issue #, if available:

Description of changes:

  • Added max and min aggregations and corresponding tests

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dai-chen
Copy link
Member

dai-chen commented Jul 13, 2020 •
edited

Could you also check if old issues like #227 can be fixed here?

@chloe-zh
support string in max and min
58a5082
@chloe-zh
Merge remote-tracking branch 'upstream/develop' into agg
0828ef1 
# Conflicts:
#	core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/SumAggregator.java
#	core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/AvgAggregatorTest.java
#	core/src/test/java/com/amazon/opendistroforelasticsearch/sql/expression/aggregation/SumAggregatorTest.java
@chloe-zh
Copy link
Member Author

Could you also check if old issues like #227 can be fixed here?

Yes, added new signature to support MIN and MAX with string. Thanks!

@dai-chen
Copy link
Member

Could you also check if old issues like #227 can be fixed here?

Yes, added new signature to support MIN and MAX with string. Thanks!

Awesome! Let's resolve old issues if fixed in new engine. Thanks.

@chloe-zh chloe-zh requested a review from dai-chen July 17, 2020 19:37
@chloe-zh chloe-zh self-assigned this Jul 17, 2020
@coderReview
Copy link

coderReview commented Jul 22, 2020 •
edited

hello @dai-chen and @chloe-zh does this PR also allow min/max for Date fields? I think DSL supports that already.

@dai-chen
Copy link
Member

hello @dai-chen and @chloe-zh does this PR also allow min/max for Date fields? I think DSL supports that already.

Thanks for your comment! I think only min/max for string is covered additionally in this PR. FYI, in this new query engine, we haven't pushed down the aggregation to Elasticsearch DSL yet. The aggregation is implemented and happening in memory for now. @chloe-zh is it possible to support date too?

@coderReview
Copy link

Don't need to that in memory. Here https://discuss.elastic.co/t/min-date-and-max-date/125174

@dai-chen
Copy link
Member

Don't need to that in memory. Here https://discuss.elastic.co/t/min-date-and-max-date/125174

Thanks for the info! Yeah, just to clarify, the post-processing in memory capability is to make sure our new engine can handle queries that unable to push down to ES DSL simply. For example, in future, we may support min/max aggregation on result of another subquery.

I think it's good to consider date support at this moment because our current aggregate operator is using JDK Math.min/max which assumes numeric input. Actually it could be generalized to support all data type that implements Comparable interface.

@chloe-zh
Merge remote-tracking branch 'upstream/develop' into agg
9796b09 
# Conflicts:
#	core/src/main/java/com/amazon/opendistroforelasticsearch/sql/expression/function/BuiltinFunctionName.java
@chloe-zh
update
0cb7af2
@chloe-zh
Merge remote-tracking branch 'upstream/develop' into agg
6ff06f2 
# Conflicts:
#	integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/StatsCommandIT.java
@chloe-zh
added min/max support for date, datetime, time, timestamp types
6ec824f
@chloe-zh
Copy link
Member Author

Hi @coderReview @dai-chen , this pr supports min/max aggregation date and time types now

@chloe-zh chloe-zh requested a review from dai-chen August 27, 2020 20:29
@chloe-zh
address comments
fcb3d24
@chloe-zh
update
d01d6d6
@chloe-zh
Merge remote-tracking branch 'upstream/develop' into agg
a2586b0 
# Conflicts:
#	integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/StatsCommandIT.java
#	sql/src/main/antlr/OpenDistroSQLParser.g4
dai-chen
dai-chen approved these changes Sep 24, 2020
View reviewed changes
Copy link
Member

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

LGTM. Could you also add some comparison test from SQL side?

@chloe-zh
Copy link
Member Author

LGTM. Could you also add some comparison test from SQL side?

Done, thanks!

@penghuo
Copy link
Contributor

penghuo commented Sep 30, 2020

Please update the docs.
https://github.com/opendistro-for-elasticsearch/sql/blob/develop/docs/experiment/ppl/cmd/stats.rst

@chloe-zh
Copy link
Member Author

Please update the docs.
https://github.com/opendistro-for-elasticsearch/sql/blob/develop/docs/experiment/ppl/cmd/stats.rst

Done, thanks!

penghuo
penghuo approved these changes Sep 30, 2020
View reviewed changes
Copy link
Contributor

@penghuo penghuo 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 change.

@chloe-zh chloe-zh merged commit a0e9b5c into opendistro-for-elasticsearch:develop Sep 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@penghuo penghuo penghuo approved these changes

@dai-chen dai-chen dai-chen approved these changes

Assignees

@chloe-zh chloe-zh

Labels
PPL SQL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chloe-zh @dai-chen @coderReview @penghuo