Replace Expression with RowExpression in assignment by hellium01 · Pull Request #12747 · prestodb/presto

hellium01 · 2019-04-30T16:22:02Z

A update on top of #12715. Will squash after review. Some utility was borrowed from another PR which will be removed once rebased on top of each other.

highker

Just did a high-level skim. This is a promising patch! No fundamental change at the moment. Vanishing the two Assignments.Builder commits should just work.

presto-main/src/main/java/com/facebook/presto/util/GraphvizPrinter.java

highker · 2019-05-02T19:35:19Z

presto-main/src/main/java/com/facebook/presto/sql/relational/SqlToRowExpressionTranslator.java

I thought about this as well. I'm just not sure originally where SymbolReference got replaced by InList. Could you add a comment here to explain?

The related test is testDoubleNestedCorrelatedSubqueries. The query (nested correlated subqueries) in the test

SELECT orderkey FROM orders o WHERE 3 IN (SELECT o.custkey FROM lineitem l WHERE (SELECT l.orderkey = o.orderkey))

is not supported today (tested in master branch). Let me try to remove this from translator and do not do a expression to row expression translation for this test. We need to think how to fix this test in longer term though.

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

presto-main/src/main/java/com/facebook/presto/sql/planner/planPrinter/PlanPrinter.java

presto-main/src/main/java/com/facebook/presto/sql/planner/plan/Assignments.java

highker · 2019-05-02T19:43:32Z

presto-main/src/main/java/com/facebook/presto/sql/relational/Expressions.java

highker · 2019-05-02T19:44:43Z

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

Make the util (with its tests) as a separate commit "Introduce RowExpressionSymbolInliner".

highker · 2019-05-02T19:46:58Z

.../main/java/com/facebook/presto/sql/planner/iterative/rule/PushProjectionThroughExchange.java

An optimizer rule should only handle Expression; otherwise it's bug. remove the else branch.

This commit might not be named correctly. It is update mainly because the test was updated to use RowExpression. Given we are migrating Expression to RowExpression. It should be only beneficial if we handle RowExpression as well here?

Then use castToRowExpression in the test. Check TestEffectivePredicateExtractor

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

highker · 2019-05-02T19:48:42Z

presto-main/src/test/java/com/facebook/presto/sql/planner/iterative/rule/test/PlanBuilder.java

There are helpers available in StandardFunctionResolution

highker

"Extract ApplyNode::isSupportedSubqueryExpression to uility" does not compile

presto-main/src/main/java/com/facebook/presto/sql/relational/SqlToRowExpressionTranslator.java

presto-main/src/test/java/com/facebook/presto/cost/TestCostCalculator.java

highker

Can we keep the momentum of this patch and #12710? Even these two patches are blocked by variable refactoring, it's good to address logic errors earlier.

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

presto-main/src/test/java/com/facebook/presto/cost/TestCostCalculator.java

highker

Reviewed all commits up to "Remove unused getOutputSymbols" (incl.). Two major comments:

The change of ValidateDependenciesChecker is a bit concerning. What is the motivation for that? In general we should fix the codebase or rule rather than relaxing the check.
The timestamps for commits are out of order. Could you fix it?

presto-main/src/test/java/com/facebook/presto/cost/TestCostCalculator.java

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

highker

"Move identity functions into AssignmentUtils" LGTM

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/InlineProjections.java

highker · 2019-06-17T23:42:58Z

btw could you also resolve the conflicts?

checkDependencies checks if referenced variables in assignment are same as input. If the assignment is type only cast (for example cast varchar(3) to varchar(5)), the referenced variable will be implicitly changed to target type. Reading symbol1, VARCHAR(3) input into symbol1 VARCHAR(5) still should be OK. Simple containsAll works when input is considered as symbol will fail when we add in the type checks.

We don't support correlated subquery with multiple correlated columns. Instead expect it to return a invalid plan, expect an exception.

highker

LGTM % minor comments; make sure to clean up SymbolsExtractor

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

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

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

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

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

facebook-github-bot added the CLA Signed label Apr 30, 2019

This was referenced May 1, 2019

Replace Assignments::Expression with RowExpression #12715

Closed

PlanNode Expression-to-RowExpression Migration Plan #12546

Closed

hellium01 force-pushed the FixProject branch 5 times, most recently from 69cd57d to 4f21a30 Compare May 2, 2019 02:38

hellium01 changed the title ~~[WIP] Use RowExpression in assignment~~ Use RowExpression in assignment May 2, 2019

hellium01 requested a review from highker May 2, 2019 16:26

hellium01 changed the title ~~Use RowExpression in assignment~~ Replace Expression with RowExpression in assignment May 2, 2019

hellium01 force-pushed the FixProject branch from 4f21a30 to ce454a0 Compare May 2, 2019 16:32

highker reviewed May 2, 2019

View reviewed changes

hellium01 force-pushed the FixProject branch 3 times, most recently from fc18e24 to 4a641bb Compare May 15, 2019 02:41

highker mentioned this pull request May 15, 2019

Move Assignments.isIdentity to ProjectNodeUtils #12603

Closed

hellium01 force-pushed the FixProject branch from 4a641bb to 318ede9 Compare May 15, 2019 16:50

hellium01 requested a review from highker May 15, 2019 16:51

hellium01 force-pushed the FixProject branch from 318ede9 to 8059c60 Compare May 15, 2019 17:53

highker reviewed May 16, 2019

View reviewed changes

presto-main/src/main/java/com/facebook/presto/sql/relational/SqlToRowExpressionTranslator.java Outdated Show resolved Hide resolved

presto-main/src/test/java/com/facebook/presto/cost/TestCostCalculator.java Outdated Show resolved Hide resolved

hellium01 force-pushed the FixProject branch 3 times, most recently from e9f657e to edf2897 Compare May 16, 2019 21:10

highker mentioned this pull request May 20, 2019

Replace ProjectNode::Expression with RowExpression step 1 #12618

Closed

highker reviewed May 21, 2019

View reviewed changes

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

presto-main/src/test/java/com/facebook/presto/cost/TestCostCalculator.java Outdated Show resolved Hide resolved

hellium01 force-pushed the FixProject branch 3 times, most recently from 7c5fa69 to 97e88ef Compare May 22, 2019 06:48

hellium01 requested a review from highker May 22, 2019 19:15

hellium01 force-pushed the FixProject branch 6 times, most recently from 869bab4 to 5e54d7a Compare June 17, 2019 20:22

hellium01 requested a review from highker June 17, 2019 20:42

highker mentioned this pull request Jun 17, 2019

Apply connector-specified optimization in PlanOptimizers #12920

Merged

hellium01 mentioned this pull request Jun 17, 2019

Move translateExpressions below HashGenerationOptimizer #12960

Merged

highker reviewed Jun 17, 2019

View reviewed changes

presto-main/src/test/java/com/facebook/presto/cost/TestCostCalculator.java Outdated Show resolved Hide resolved

...o-main/src/main/java/com/facebook/presto/sql/planner/sanity/ValidateDependenciesChecker.java Show resolved Hide resolved

highker self-requested a review June 17, 2019 22:29

highker reviewed Jun 17, 2019

View reviewed changes

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/InlineProjections.java Outdated Show resolved Hide resolved

highker self-requested a review June 18, 2019 00:23

hellium01 and others added 11 commits June 17, 2019 17:24

Extract ApplyNode::isSupportedSubqueryExpression to utility

0abe624

Translate row expresion before fragment it in TestCostCalculator

f3eb170

Catch exception in test unsupported subquery

d419732

We don't support correlated subquery with multiple correlated columns. Instead expect it to return a invalid plan, expect an exception.

Fix RowExpressionVerifier cast assertion

60d1f34

Move toVariableReference method into PlannerUtils

f0d87e5

Remove unused getOutputSymbols

c19cfad

Move identity functions into AssignmentUtils

a034b1f

Replace Expression in Assignments to RowExpression

0463baa

Remove OriginalExpression before plan assertion

09c5b45

Remove unused assertPlan

2032ae4

hellium01 force-pushed the FixProject branch from 5e54d7a to 2032ae4 Compare June 18, 2019 00:25

highker approved these changes Jun 18, 2019

View reviewed changes

hellium01 merged commit ad7382b into prestodb:master Jun 18, 2019

hellium01 deleted the FixProject branch June 18, 2019 20:58

hellium01 mentioned this pull request Jun 18, 2019

Fixup unaddressed comments #12967

Merged

Conversation

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

Uh oh!

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

highker commented Jun 17, 2019

Uh oh!

highker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hellium01 commented Apr 30, 2019 •

edited

Loading