[CALCITE-6109] Avoid extra loops when optimizing statements with ternary expressions #3518
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mihaibudiu
approved these changes
Nov 12, 2023
| @@ -152,6 +154,25 @@ void checkAssignInConditionOptimizedOut(int modifiers, String s) { | |||
| assertThat(Expressions.toString(builder.toBlock()), is(s)); | |||
| } | |||
|
|
|||
| @Test void testInlineTernaryNonOptimized() { | |||
There was a problem hiding this comment.
I think it's customary to add a Javadoc comment indicating the issue which is being solved. Please follow the format of the many examples existing, the tools are very particular about the format, including the final period.
There was a problem hiding this comment.
I think it's customary to add a Javadoc comment indicating the issue which is being solved. Please follow the format of the many examples existing, the tools are very particular about the format, including the final period.
Thanks for your suggestion, I have added Javadoc comment on the test case.
|
This seems to affect the output of at least one Druid test. |
JiajunBernoulli
added
the
LGTM-will-merge-soon
|
Failed tests are not related to changes. Please rerun the CI. |
JiajunBernoulli
removed
the
LGTM-will-merge-soon
asdfgh19
force-pushed
the
CALCITE-6109
branch
from
280d0fd
to
7a6db03
Compare
Thanks for helping to review the code. |
|
Oops! Druid Tests Failed again.
|
asdfgh19
force-pushed
the
CALCITE-6109
branch
2 times, most recently
from
6fa88a3
to
70c3a62
Compare
I saw the same error in some other PRs, and after I rerun the CI, the tests passed. |
|
@asdfgh19 |
OK, I will try to working on it latter. |
julianhyde
reviewed
Nov 14, 2023
| Expressions.parameter(int.class, "c"))); | ||
| b.add(originStatement); | ||
| BlockStatement block = b.toBlock(); | ||
| assertEquals(block.statements.size(), 1); |
There was a problem hiding this comment.
use assertThat rather than assertEquals and assertSame. people get the args in the wrong order. as you did.
There was a problem hiding this comment.
Do you mean that assertSame and assertEquals easily make people confused which is expected and which is actual? So I need to change to assertThat, right?
julianhyde
reviewed
Nov 14, 2023
| b.add(originStatement); | ||
| BlockStatement block = b.toBlock(); | ||
| assertEquals(block.statements.size(), 1); | ||
| assertSame(originStatement, block.statements.get(0)); |
There was a problem hiding this comment.
why you require assertSame is rather subtle. and it's the whole point of the test. so, say why.
There was a problem hiding this comment.
Because the Javadoc comment of the BlockBuilder#optimize method indicates that this method returns whether any optimizations were made, and the basis for its judgment is whether the original statement and the statement optimized by InlineVariableVisitor and OptimizeShuttle are the same instance.
Moreover, in the Shuttle class, all visit methods will check the corresponding statement. If the fields of the statement have not been modified, the original statement will be returned.
So I wrote this test case as simple as possible, which only includes one line of code that ensures the conventions of the BlockBuilder and Shuttle classes.
asdfgh19
force-pushed
the
CALCITE-6109
branch
from
70c3a62
to
766c91b
Compare
|
Kudos, SonarCloud Quality Gate passed! |
asdfgh19
force-pushed
the
CALCITE-6109
branch
from
766c91b
to
94f6819
Compare
julianhyde
added a commit
to julianhyde/calcite
that referenced
this pull request
Nov 17, 2023
… of TernaryExpression if it does not do any optimization Before this change, OptimizeShuttle might return a different Expression object that is equivalent to the original; this would cause the optimizer to loop, mistakenly believing that optimizations were occurring, which was inefficient. After this change, the OptimizeShuttle avoids extra loops when optimizing statements with ternary expressions. Close apache#3518
github-actions bot
pushed a commit
to julianhyde/calcite
that referenced
this pull request
Nov 17, 2023
… of TernaryExpression if it does not do any optimization Before this change, OptimizeShuttle might return a different Expression object that is equivalent to the original; this would cause the optimizer to loop, mistakenly believing that optimizations were occurring, which was inefficient. After this change, the OptimizeShuttle avoids extra loops when optimizing statements with ternary expressions. Close apache#3518
julianhyde
added a commit
to julianhyde/calcite
that referenced
this pull request
Nov 20, 2023
… of TernaryExpression if it does not do any optimization Before this change, OptimizeShuttle might return a different Expression object that is equivalent to the original; this would cause the optimizer to loop, mistakenly believing that optimizations were occurring, which was inefficient. After this change, the OptimizeShuttle avoids extra loops when optimizing statements with ternary expressions. Close apache#3518
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
jira: CALCITE-6109
When there are statements with ternary expressions in BlockBuilder#statements, the optimize method always returns true, causing the loop in the toBlock method to always be executed 10 times.
The reason is that when OptimizeShuttle traverses the statement, a new instance of TernaryExpression will always be created, regardless of whether the optimization is actually performed.