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

Simplify coalesce expressions #12192

Closed
wants to merge 1 commit into from
Closed

Simplify coalesce expressions #12192

wants to merge 1 commit into from

Conversation

Praveen2112
Copy link
Contributor

For coalesce expression like coalesce(colA, colA) can be simplified to colA and for expressions like coalesce('1', colA) can be simplified to '1'. This patch captures those pattern and simplifies them. Also updated the test for the above changes

@Praveen2112 Praveen2112 force-pushed the simplify_coalesce_expression branch from 0cb5678 to 7f0e4ad Compare January 8, 2019 10:51
@Praveen2112
Copy link
Contributor Author

Praveen2112 commented Jan 8, 2019 •
edited
Loading

@kokosing Have added support for nested coalesce expressions.

@Praveen2112 Praveen2112 force-pushed the simplify_coalesce_expression branch from 7f0e4ad to 3dd19a7 Compare January 8, 2019 12:01
@kokosing
Copy link
Contributor

kokosing commented Jan 8, 2019

@sopel39 Would you like to take a look also?

sopel39
sopel39 reviewed Jan 8, 2019
View reviewed changes
presto-main/src/main/java/com/facebook/presto/sql/planner/ExpressionInterpreter.java
@@ -546,19 +548,38 @@ protected Object visitCoalesceExpression(CoalesceExpression node, Object context
List<Object> values = node.getOperands().stream()
.map(value -> processWithExceptionHandling(value, context))
.filter(Objects::nonNull)
.flatMap(expression -> {
if (expression instanceof CoalesceExpression) {
return ((CoalesceExpression) expression).getOperands().stream();
Copy link
Contributor

Choose a reason for hiding this comment

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

what if there are more levels of recursion? Make it recursive

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the current model is recursive right. We have added tests with more levels of coalesce and it passed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It only supports one level of recursion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't ExpressionInterpreter#processWithExceptionHandling will call other levels ? We just need to check if one of argument is of CoalesceExpression If yes we will extract its arguments and push it to the outer expression and processWithExceptionHandling will ideally handle at more levels

If I am not wrong recursive implementation will simplify
coalesce(coalesce(unbound_long, coalesce(unbound_long, 1)), unbound_long2) to coalesce(unbound_long, 1)

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I have replaced it with isDeterministic((CoalesceExpression) expression)

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean I think that isDeterministic((CoalesceExpression) expression) can be removed completely. We don't need to check if nested coalesce is deterministic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in that case it will simplify coalesce(unbound_long, coalesce(random(), 1) to coalesce(unbound_long, random(), 1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sopel39 Removed the deterministic check as it is not required now

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you. Ping us when release is done to land it

presto-main/src/main/java/com/facebook/presto/sql/planner/ExpressionInterpreter.java Outdated Show resolved Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/ExpressionInterpreter.java Outdated Show resolved Hide resolved
@findepi
Copy link
Contributor

findepi commented Jan 8, 2019

@Praveen2112 this will help a bit with #12155 (as per #12155 (comment)). Nice

@Praveen2112
Copy link
Contributor Author

@findepi I guess this one #12193 would also help the above issue.

@findepi findepi mentioned this pull request Jan 8, 2019
Closed
@Praveen2112 Praveen2112 force-pushed the simplify_coalesce_expression branch from 3dd19a7 to fe3ef94 Compare January 8, 2019 17:14
sopel39
sopel39 approved these changes Jan 9, 2019
View reviewed changes
presto-main/src/main/java/com/facebook/presto/sql/planner/ExpressionInterpreter.java
@@ -546,19 +548,38 @@ protected Object visitCoalesceExpression(CoalesceExpression node, Object context
List<Object> values = node.getOperands().stream()
.map(value -> processWithExceptionHandling(value, context))
.filter(Objects::nonNull)
.flatMap(expression -> {
if (expression instanceof CoalesceExpression) {
return ((CoalesceExpression) expression).getOperands().stream();
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, thanks

presto-main/src/main/java/com/facebook/presto/sql/planner/ExpressionInterpreter.java Show resolved Hide resolved
@Praveen2112 Praveen2112 force-pushed the simplify_coalesce_expression branch 2 times, most recently from 6f1e657 to 0993125 Compare January 9, 2019 17:27
@Praveen2112 Praveen2112 mentioned this pull request Jan 10, 2019
Closed
@Praveen2112 Praveen2112 force-pushed the simplify_coalesce_expression branch 2 times, most recently from b843fa2 to 8fc124b Compare January 11, 2019 17:00
@Praveen2112 Praveen2112 force-pushed the simplify_coalesce_expression branch from 8fc124b to 2803ca6 Compare January 12, 2019 00:26
@Praveen2112 Praveen2112 mentioned this pull request Jan 24, 2019
Merged
@highker highker self-assigned this Feb 19, 2019
@highker
Copy link
Contributor

highker commented Feb 23, 2019

Merged

@highker highker closed this Feb 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sopel39 sopel39 sopel39 approved these changes

@findepi findepi findepi left review comments

@martint martint martint left review comments

@electrum electrum electrum left review comments

@kokosing kokosing kokosing approved these changes

Assignees

@highker highker

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Praveen2112 @kokosing @findepi @highker @electrum @martint @sopel39 @facebook-github-bot