Flatten FunctionCall as direct fields in AggregationNode#Aggregation by hellium01 · Pull Request #12789 · prestodb/presto

hellium01 · 2019-05-10T18:52:30Z

This is the first 3 commits of #12710

presto-main/src/main/java/com/facebook/presto/sql/analyzer/QueryExplainer.java

presto-main/src/main/java/com/facebook/presto/sql/planner/DesugarAtTimeZoneRewriter.java

...ain/java/com/facebook/presto/sql/planner/iterative/rule/PushAggregationThroughOuterJoin.java

presto-main/src/main/java/com/facebook/presto/sql/planner/ExpressionExtractor.java

highker · 2019-05-11T23:57:31Z

presto-main/src/main/java/com/facebook/presto/sql/planner/sanity/TypeValidator.java

Add ticket number to this

Do we need a ticket number since it will be added back in following commit.

...java/com/facebook/presto/sql/planner/iterative/rule/TestPruneCountAggregationOverScalar.java

...java/com/facebook/presto/sql/planner/iterative/rule/TestPushAggregationThroughOuterJoin.java

...ain/src/test/java/com/facebook/presto/sql/planner/assertions/AggregationFunctionMatcher.java

presto-main/src/main/java/com/facebook/presto/sql/planner/PlannerUtils.java

highker

LGTM

presto-main/src/test/java/com/facebook/presto/sql/planner/TestTypeValidator.java

highker · 2019-05-14T06:48:44Z

presto-main/src/main/java/com/facebook/presto/sql/planner/AnalyzedExpressionRewriter.java

Add @SuppressWarnings("unchecked") if you want; but not necessary.

wenleix

"Add utility functions to convert SortOrder and OrderingScheme": LGTM, one nit.

presto-main/src/main/java/com/facebook/presto/sql/planner/PlannerUtils.java

wenleix

"Add standard aggregation to StandardFunctionResolution". LGTM.

I remember seeing discussions to have them, just want to remember myself why we need them? (probably in a function related PR that @rongrong and @highker discussed? )

hellium01 · 2019-05-14T16:58:31Z

Thanks for reviewing, yes, I can use Expression instead of template in AnalyzedExpressionRewriter (will update that).

In optimization rules, we will likely to add some standard aggregation functions (one example place is StatisticAggregation in TableWriter) like count, max, min, adding them to FunctionResolution allows us to do that.

wenleix

"Flatten FunctionCall as direct fields in AggregationNode#Aggregation" . Some comments.

presto-main/src/main/java/com/facebook/presto/sql/planner/AnalyzedExpressionRewriter.java

wenleix · 2019-05-14T16:58:03Z

presto-main/src/main/java/com/facebook/presto/sql/planner/AnalyzedExpressionRewriter.java

hmm... AnalyzedExpressionRewriter sounds a bit vague... let's talk in person quickly to figure out a better name (or decided that's the best name and we just need some comments.. 😃 )

As discussed offline, will add a Deprecated Mark over there and keep the name as is.

...rc/main/java/com/facebook/presto/sql/planner/iterative/rule/AddIntermediateAggregations.java

...o-main/src/main/java/com/facebook/presto/sql/planner/optimizations/AggregationNodeUtils.java

wenleix · 2019-05-14T18:50:55Z

presto-main/src/main/java/com/facebook/presto/sql/planner/DesugarAtTimeZoneRewriter.java

hmmm.. should this expressionType be served as context? Let's talk in person.

As discussed offline, will update to use a RewriterProvider

wenleix

The refactor into AnalyzedExpressionRewriter part looks good. I didn't look deep into the "flatten" logic part since it get reviewed in #12710

presto-main/src/main/java/com/facebook/presto/sql/planner/AnalyzedExpressionRewriter.java

Previously these information is stored in `FunctionCall`, which is an `Expression`. This is required by following commits which will make AggregationNode to use `RowExpression` instead of `Expression`.

facebook-github-bot added the CLA Signed label May 10, 2019

hellium01 requested a review from highker May 10, 2019 18:52

hellium01 mentioned this pull request May 10, 2019

Make Aggregation use RowExpression instead #12710

Merged

highker reviewed May 12, 2019

View reviewed changes

highker requested a review from wenleix May 12, 2019 00:35

hellium01 force-pushed the FlattenAgg branch 4 times, most recently from 5a86a40 to b108ac8 Compare May 13, 2019 20:12

highker self-requested a review May 14, 2019 05:57

highker approved these changes May 14, 2019

View reviewed changes

wenleix reviewed May 14, 2019

View reviewed changes

presto-main/src/main/java/com/facebook/presto/sql/planner/PlannerUtils.java Outdated Show resolved Hide resolved

wenleix reviewed May 14, 2019

View reviewed changes

hellium01 force-pushed the FlattenAgg branch from b108ac8 to ed68241 Compare May 14, 2019 17:03

hellium01 added 2 commits May 14, 2019 10:03

Add utility functions to convert SortOrder and OrderingScheme

8bfc1a1

Add standard aggregation to StandardFunctionResolution

1a580ae

hellium01 force-pushed the FlattenAgg branch from ed68241 to a3052b8 Compare May 14, 2019 17:03

wenleix reviewed May 14, 2019

View reviewed changes

hellium01 force-pushed the FlattenAgg branch 5 times, most recently from 86ab56a to 92ab142 Compare May 14, 2019 21:25

hellium01 requested a review from wenleix May 14, 2019 21:35

wenleix approved these changes May 14, 2019

View reviewed changes

presto-main/src/main/java/com/facebook/presto/sql/planner/AnalyzedExpressionRewriter.java Outdated Show resolved Hide resolved

presto-main/src/main/java/com/facebook/presto/sql/planner/AnalyzedExpressionRewriter.java Outdated Show resolved Hide resolved

Flatten FunctionCall as direct fields in AggregationNode#Aggregation

5063c69

Previously these information is stored in `FunctionCall`, which is an `Expression`. This is required by following commits which will make AggregationNode to use `RowExpression` instead of `Expression`.

hellium01 force-pushed the FlattenAgg branch from 92ab142 to 5063c69 Compare May 14, 2019 23:15

hellium01 merged commit 13e1d0b into prestodb:master May 15, 2019

hellium01 deleted the FlattenAgg branch May 15, 2019 01:52

highker mentioned this pull request May 23, 2019

Incorrect results when use_mark_distinct = false is set #12860

Closed

Conversation

hellium01 commented May 10, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

highker May 11, 2019

Choose a reason for hiding this comment

Uh oh!

hellium01 May 13, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

highker May 14, 2019

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

hellium01 commented May 14, 2019

Uh oh!

wenleix left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wenleix May 14, 2019

Choose a reason for hiding this comment

Uh oh!

hellium01 May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wenleix May 14, 2019

Choose a reason for hiding this comment

Uh oh!

hellium01 May 14, 2019

Choose a reason for hiding this comment

Uh oh!

wenleix left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hellium01 May 14, 2019 •

edited

Loading

wenleix left a comment •

edited

Loading