Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move row expression above last projectionPushDown #13047

Merged
merged 7 commits into from
Aug 22, 2019
Merged

Move row expression above last projectionPushDown #13047

merged 7 commits into from
Aug 22, 2019

Conversation

hellium01
Copy link
Contributor

@hellium01 hellium01 commented Jul 3, 2019 •
edited
Loading 

Release note
-------
no user affecting change.

On top of #12982

@hellium01 hellium01 force-pushed the MoveRowExpressionUpStep3 branch 2 times, most recently from bc448d1 to 951ae35 Compare July 9, 2019 19:19
@hellium01 hellium01 force-pushed the MoveRowExpressionUpStep3 branch from 951ae35 to e0a3638 Compare July 19, 2019 18:29
@hellium01 hellium01 force-pushed the MoveRowExpressionUpStep3 branch 11 times, most recently from 1453b67 to d022a19 Compare August 6, 2019 00:41
@hellium01 hellium01 mentioned this pull request Aug 6, 2019
Closed
@hellium01 hellium01 force-pushed the MoveRowExpressionUpStep3 branch from d022a19 to cc7e6d3 Compare August 6, 2019 17:28
@hellium01 hellium01 changed the title [WIP] Move row expression up step3 Move row expression above last projectionPushDown Aug 6, 2019
@hellium01 hellium01 mentioned this pull request Aug 6, 2019
Merged
@hellium01 hellium01 force-pushed the MoveRowExpressionUpStep3 branch from cc7e6d3 to 2d7c1c6 Compare August 8, 2019 23:59
@hellium01 hellium01 requested a review from highker August 9, 2019 01:27
@highker highker requested a review from rongrong August 9, 2019 19:35
highker
highker reviewed Aug 9, 2019
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

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

some early comments

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/PlanAssert.java Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/InlineProjections.java Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/InlineProjections.java Outdated Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/InlineProjections.java Outdated Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/InlineProjections.java Outdated Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/InlineProjections.java Outdated Show resolved Hide resolved
...ain/src/main/java/com/facebook/presto/sql/planner/optimizations/UnaliasSymbolReferences.java Outdated Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/Util.java Outdated Show resolved Hide resolved
highker
highker reviewed Aug 9, 2019
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

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

Finished review. LGTM % minor issues

presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/InlineProjections.java Outdated Show resolved Hide resolved
...ain/src/main/java/com/facebook/presto/sql/planner/optimizations/UnaliasSymbolReferences.java Outdated Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/RowExpressionVariableInliner.java Show resolved Hide resolved
@hellium01 hellium01 force-pushed the MoveRowExpressionUpStep3 branch from 2d7c1c6 to 714b6e0 Compare August 14, 2019 21:10
@hellium01 hellium01 requested a review from highker August 14, 2019 21:32
highker
highker approved these changes Aug 14, 2019
View reviewed changes
Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

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

LGTM % nits

presto-main/src/test/java/com/facebook/presto/sql/planner/assertions/PlanAssert.java Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/InlineProjections.java Outdated Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/Util.java Outdated Show resolved Hide resolved
@hellium01 hellium01 force-pushed the MoveRowExpressionUpStep3 branch 3 times, most recently from 6f3a493 to e410d0a Compare August 14, 2019 22:29
@hellium01 hellium01 force-pushed the MoveRowExpressionUpStep3 branch from e410d0a to 98c47aa Compare August 14, 2019 23:30
@hellium01 hellium01 force-pushed the MoveRowExpressionUpStep3 branch from 98c47aa to a9cc91a Compare August 15, 2019 00:10
@hellium01 hellium01 requested a review from rongrong August 15, 2019 00:11
@hellium01 hellium01 force-pushed the MoveRowExpressionUpStep3 branch 2 times, most recently from 16ccae9 to 1feea40 Compare August 15, 2019 00:31
@hellium01
Copy link
Contributor Author

Thanks for reviewing! Updated to the last comments.

@hellium01 hellium01 force-pushed the MoveRowExpressionUpStep3 branch from 1feea40 to 3a89a52 Compare August 17, 2019 01:03
@highker
Copy link
Contributor

highker commented Aug 21, 2019

Is this good to merge? Assign to me if it is.

@rongrong
Copy link
Contributor

Consider that these refactoring has triggered some bugs in previous releases, do we want to run some independent verifier tests before merging?

@hellium01
Copy link
Contributor Author

Yes, verifier tests passed with 4000 queries.

@highker highker merged commit ffe309d into prestodb:master Aug 22, 2019
@neeradsomanchi neeradsomanchi mentioned this pull request Sep 9, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@highker highker highker approved these changes

@rongrong rongrong Awaiting requested review from rongrong

Assignees

@highker highker

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hellium01 @highker @rongrong @facebook-github-bot