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.

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

[CALCITE-3845] CASE WHEN expression with nullability CAST is considered as reduced wrongly in ReduceExpressionsRule #1848

Merged
merged 1 commit into from Mar 10, 2020

Conversation

chunweilei
Copy link
Contributor

@chunweilei chunweilei commented Mar 6, 2020

@chunweilei chunweilei force-pushed the CALCITE-3845 branch 2 times, most recently from 28d7154 to 0746873 Compare March 7, 2020 05:36
Copy link
Member

@hsyuan hsyuan left a comment

Choose a reason for hiding this comment

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

+1

@DonnyZone
Copy link
Contributor

LGTM

@chunweilei chunweilei added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Mar 9, 2020
@@ -302,6 +302,7 @@ public ProjectReduceExpressionsRule(Class<? extends Project> projectClass,
Lists.newArrayList(project.getProjects());
if (reduceExpressions(project, expList, predicates, false,
matchNullability)) {
assert !project.getProjects().equals(expList);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add a message, so the failure looks human-readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Will do.

Comment on lines +3499 to +3506
final RexNode caseRexNode = rexBuilder.makeCall(SqlStdOperatorTable.CASE,
rexBuilder.makeCall(SqlStdOperatorTable.EQUALS,
rexBuilder.makeCall(SqlStdOperatorTable.MOD, ref, literal2), literal1),
rexBuilder.makeCall(SqlStdOperatorTable.LESS_THAN, ref, literal2),
rexBuilder.makeCall(SqlStdOperatorTable.EQUALS,
rexBuilder.makeCall(SqlStdOperatorTable.MOD, ref, literal3), literal2),
rexBuilder.makeCall(SqlStdOperatorTable.LESS_THAN, ref, literal1),
rexBuilder.makeCall(SqlStdOperatorTable.LESS_THAN, ref, literal3));
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks way too verbose :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wish I can provide a more simple case, but I cannot. Because CASE WHEN will be changed to OR in many cases and thus it will not reproduce the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is signal to noise ratio leaves much to be desired here.
The dance of rexBuilder.makeCall(SqlStdOperatorTable repeats again and again :(

…ed as reduced wrongly in ReduceExpressionsRule
@chunweilei chunweilei merged commit ad8cf7e into apache:master Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM-will-merge-soon Overall PR looks OK. Only minor things left.
Projects
None yet
4 participants