Make Aggregation use RowExpression instead

[hellium01]

This will remove the dependency to Expression in following plan nodes:

AggregationNode
TableWriterNode
TableFinishNode

It is splitted to small commits that may need to squash together later.

[highker]

@hellium01 is this ready to review?

[hellium01]

Yes, it should be OK to review now. It was splitted to small commits but I can squash them after review.

[highker]

There are many places still having both getCall and the other new getters. To make it clean, we'd better clean them in one commit.

[highker]

Put this to the previous line

[hellium01]

It will be a little long to fit in one line.

[highker]

"Change expressionExtractor for aggregation" done

[highker]

Why don't we use function manager?

[highker]

Finished the first 3 commits. I think we need some care to make sure refactoring can be easily carried out. Moving arguments out of FunctionCall may not be proper. It loosens the bound as a function. We need to keep the basic signature of a function as a whole and separating out other info like order by or distinct. Keeping arguments inside FunctionCall can help replacing it with CallExpression with one shot. Also, it won't introduce the problem of resolving lambdas.

[wenleix]

Glance over commits through "Format aggregation in explainer" . I leave detailed review to @highker 😃

[hellium01]

Rebased on top #12606

[wenleix]

"Modify RowExpressionSymbolInliner to RowExpressionVariableInliner": LGTM % one nit.

[wenleix]

"Consolidate conversion from OrderBy expression to OrderingScheme".

LGTM % nits.

[wenleix]

"Replace Expression with RowExpression in Aggregation".

Skimmed through since most of the comments are discussed in previous reviews. @highker and @rongrong can have a more detailed review on optimizer and expression rewrite related stuff.

[highker]

"Modify RowExpressionSymbolInliner to RowExpressionVariableInliner" LGTM

[highker]

"Replace Expression with RowExpression in Aggregation" minor comments only

[highker]

let's move this part of logic to the caller. The caller should decide which method to use.

[hellium01]

SymbolMapper will be used in multiple optimization rules. As we are pushing the clean up up, we will need this function to handle both Expression/RowExpression anyway. I would prefer we just remove isExpression later once everything is cleaned up.

[highker]

Two other commits LGTM

[wenleix]

LGTM. Make sure Travis turn green before merge.

Thanks for the great work!

[mbasmanova]

@hellium01 Yi, I'm seeing Streaming aggregation with input not grouped on the grouping keys planning error for the following query. Could it be related to this change?

SELECT
      linenumber,
      'xxx'
  FROM
  (
      (SELECT orderkey, linenumber FROM lineitem)
      UNION
      (SELECT orderkey, linenumber FROM lineitem)
  ) WHERE orderkey = 1 
  GROUP BY
      1;



java.lang.IllegalArgumentException: Streaming aggregation with input not grouped on the grouping keys

    at com.google.common.base.Preconditions.checkArgument(Preconditions.java:135)
    at com.facebook.presto.sql.planner.sanity.ValidateStreamingAggregations$Visitor.visitAggregation(ValidateStreamingAggregations.java:89)
    at com.facebook.presto.sql.planner.sanity.ValidateStreamingAggregations$Visitor.visitAggregation(ValidateStreamingAggregations.java:52)
    at com.facebook.presto.sql.planner.plan.AggregationNode.accept(AggregationNode.java:200)
    at com.facebook.presto.sql.planner.plan.InternalPlanNode.accept(InternalPlanNode.java:31)
    at com.facebook.presto.sql.planner.sanity.ValidateStreamingAggregations$Visitor.lambda$visitPlan$0(ValidateStreamingAggregations.java:73)