[CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first #2237

chunweilei · 2020-10-30T08:44:50Z

Usually O_INPUTS are only applied for groups with physical convention. But
when enabling AbstractConverter, the input of AbstractConverter might be
a group with NONE convention. In that case, no need to apply O_INPUTS.
Otherwise, it might throw an exception due to impossible transformation(
physical convention -> none convention).

Other change:

some cosmetic fix-ups
print the upper bound of the RelSubSet

danny0405 · 2020-11-02T07:26:24Z

Remember to update the commit message before commit.

chunweilei · 2020-11-02T07:50:50Z

emember to update the commit message before commit.

Could you please tell me what I should update about the commit message? Thanks.

chunweilei · 2020-11-02T08:24:28Z

Updated the commit message. Thank you for your suggestion, @danny0405 .

vlsi · 2020-11-02T08:33:07Z

Frankly speaking, it is hard to review the change because there are lots of comment changes, and it is hard to spot code changes.

Please refrain from mixing unrelated comment changes and code changes into a single commit.

It does not mean I require that for this case, however, I opened this PR several times, and every time I thought it was javadoc-only change.

vlsi · 2020-11-02T08:36:35Z

Could you please tell me what I should update about the commit message? Thanks

@chunweilei , could you please clarify what is the nature of the change?
Is it a cosmetic change?
Does this PR fix a bug?
Does this PR introduce a new feature?

The commit message says [CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first, so the commit message suggests the change fixes a bug. However, the diff looks like a javadoc-only cosmetic change.

In case the PR is cosmetic, then the very same thing should be mentioned in the commit message. Otherwise, it is misleading, and it is time-consuming for the reviewers.

chunweilei · 2020-11-02T08:39:14Z

@vlsi I have updated the commit message. This change not only fixes a bug but also have some fix-ups.

core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java

chunweilei · 2020-11-02T08:46:50Z

Frankly speaking, it is hard to review the change because there are lots of comment changes, and it is hard to spot code changes.

Please refrain from mixing unrelated comment changes and code changes into a single commit.

It does not mean I require that for this case, however, I opened this PR several times, and every time I thought it was javadoc-only change.

I don't want to see many commits that only have comment changes. It is not necessary. But I should have underlined the main change. Then it is much easier for reviewers.

vlsi · 2020-11-02T08:56:29Z

@chunweilei, please split the change in two:

code change with the corresponding comments
other comment cleanups

Currently, it is hard to figure out where is the code part of the change.

chunweilei · 2020-11-02T08:56:38Z

The main change is here: https://github.com/apache/calcite/pull/2237/files#diff-ad65441646b31d60e7a55d6bac2c1f4d9ddba3473b227963800b1b2bc292a319R342

vlsi · 2020-11-02T08:58:23Z

Please add tests to cover the change

chunweilei · 2020-11-02T09:00:37Z

Please add tests to cover the change

It's difficult to add a test to reproduce it. But you can reproduce it if you change the applying order of substitution rule and non-substitution rule. I described it in the jira case.

vlsi · 2020-11-02T09:01:34Z

The main change is here: https://github.com/apache/calcite/pull/2237/files#diff-ad65441646b31d60e7a55d6bac2c1f4d9ddba3473b227963800b1b2bc292a319R342

It must come as its own commit. You should not require each and every reviewer to inspect the diff in order to find a single line change buried under lots of comment changes.

chunweilei · 2020-11-02T09:28:47Z

It must come as its own commit. You should not require each and every reviewer to inspect the diff in order to find a single line change buried under lots of comment changes.

I do not require anything from anybody. It's just about preference. Does it really so difficult that someone has to spend hours on finding the main change? I don't think so. As you can see, two people have reviewed it at least and give their comments.

chunweilei · 2020-11-02T09:31:50Z

@chunweilei, please split the change in two:

code change with the corresponding comments

other comment cleanups

Currently, it is hard to figure out where is the code part of the change.

I am open to this change. Would split the change into two soon.

… first Usually O_INPUTS are only applied for groups with physical convention. But when enabling AbstractConverter, the input of AbstractConverter might be a group with NONE convention. In that case, no need to apply O_INPUTS. Otherwise, it might throw en exception due to impossible transformation( physical convention -> none convention).

chunweilei · 2020-11-02T09:55:13Z

@chunweilei, please split the change in two:

code change with the corresponding comments

other comment cleanups

Currently, it is hard to figure out where is the code part of the change.

I have split the change into two. Thank you for your suggestion.

vlsi · 2020-11-02T10:19:23Z

core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java

@@ -335,6 +336,14 @@ default boolean onProduce(RelNode node) {
          physicals.add(rel);
        }
      }
+
+      // No need to apply O_INPUTS if the group has not been implemented yet.


What is O_INPUTS?

It means OptimzeInput, which is a term in Cascades planner. I borrow it from the comment[1].

[1] https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java#L338

I see no reasons to invent obscure words when the thing can be explained in plain English.

I would change it as you suggest. But the term O_INPUT is not invented currently. It comes from a classic paper[1] which was published in 1998.

[1] https://15721.courses.cs.cmu.edu/spring2019/papers/22-optimizer1/xu-columbia-thesis1998.pdf Page#63

vlsi · 2020-11-02T10:30:28Z

have split the change into two

Thanks. It would be better if you specify the nature and the location of the change instead of the current Some cosmetic cleanup.
For instance: Fix grammar in Volcano planner comments

It's difficult to add a test to reproduce it. But you can reproduce it if you change the applying order of substitution rule and non-substitution rule.

If the bug happens only if you manually edit Calcite source, then why do you think it is a bug after all?

I described it in the jira case.

I do not see where CALCITE-4368 describes why test can't be added.

chunweilei · 2020-11-02T11:28:37Z

If the bug happens only if you manually edit Calcite source, then why do you think it is a bug after all?

It might happen in some cases. But I could not construct one currently.
Even though we change the order of the rule, the test cases should not fail. It is not expected.

chunweilei · 2020-11-02T11:30:15Z

I do not see where CALCITE-4368 describes why test can't be added.

I mean I describe the way that reproduces the error in the JIRA case.

vlsi · 2020-11-02T11:31:03Z

I'm inclined that this change must be accompanied by a test, otherwise, it should not be committed.

chunweilei · 2020-11-02T11:41:07Z

I'm inclined that this change must be accompanied by a test, otherwise, it should not be committed.

As you insist, I would try my best to provide one.

chunweilei · 2020-11-04T02:53:54Z

Opened another PR to fix comments: #2243.

chunweilei · 2020-12-14T02:30:21Z

Close the PR since I can not reproduce it without changes.

chunweilei changed the title ~~[CALCITE-4360] TopDownOptTest fails if applying non-substitution rule first~~ [CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first Oct 30, 2020

chunweilei force-pushed the CALCITE-4368 branch from d4de7a5 to 87868f4 Compare October 30, 2020 08:46

FatLittle approved these changes Nov 2, 2020

View reviewed changes

chunweilei added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Nov 2, 2020

chunweilei force-pushed the CALCITE-4368 branch from 87868f4 to 392623d Compare November 2, 2020 07:46

chunweilei force-pushed the CALCITE-4368 branch from 392623d to f25bbc8 Compare November 2, 2020 08:16

vlsi removed the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Nov 2, 2020

chunweilei commented Nov 2, 2020

View reviewed changes

core/src/main/java/org/apache/calcite/plan/volcano/TopDownRuleDriver.java Show resolved Hide resolved

vlsi added returned-with-feedback There are review comments (in JIRA and/or in GitHub) to be implemented before merging the PR tests-missing labels Nov 2, 2020

chunweilei force-pushed the CALCITE-4368 branch from f25bbc8 to b0d2f5a Compare November 2, 2020 09:45

chunweilei removed the returned-with-feedback There are review comments (in JIRA and/or in GitHub) to be implemented before merging the PR label Nov 2, 2020

chunweilei added 2 commits November 2, 2020 17:52

Some cosmetic fix-ups

54b8d9d

chunweilei force-pushed the CALCITE-4368 branch from b0d2f5a to 54b8d9d Compare November 2, 2020 09:54

vlsi reviewed Nov 2, 2020

View reviewed changes

vlsi added the returned-with-feedback There are review comments (in JIRA and/or in GitHub) to be implemented before merging the PR label Nov 2, 2020

julianhyde force-pushed the master branch from 0e71469 to c708456 Compare November 3, 2020 00:43

chunweilei mentioned this pull request Nov 4, 2020

Fix grammatical errors in TopDownRuleDriver/TopDownRuleQueue/RuleDriver/VolcanoPlanner #2243

Merged

vlsi force-pushed the master branch from fd20efd to 3311d45 Compare December 9, 2020 20:10

chunweilei closed this Dec 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first #2237

[CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first #2237

chunweilei commented Oct 30, 2020 •

edited

danny0405 commented Nov 2, 2020

chunweilei commented Nov 2, 2020

chunweilei commented Nov 2, 2020

vlsi commented Nov 2, 2020

vlsi commented Nov 2, 2020

chunweilei commented Nov 2, 2020

chunweilei commented Nov 2, 2020 •

edited

vlsi commented Nov 2, 2020

chunweilei commented Nov 2, 2020

vlsi commented Nov 2, 2020

chunweilei commented Nov 2, 2020 •

edited

vlsi commented Nov 2, 2020

chunweilei commented Nov 2, 2020

chunweilei commented Nov 2, 2020 •

edited

chunweilei commented Nov 2, 2020

vlsi Nov 2, 2020

chunweilei Nov 2, 2020

vlsi Nov 2, 2020

chunweilei Nov 2, 2020 •

edited

vlsi commented Nov 2, 2020

chunweilei commented Nov 2, 2020

chunweilei commented Nov 2, 2020

vlsi commented Nov 2, 2020

chunweilei commented Nov 2, 2020

chunweilei commented Nov 4, 2020

chunweilei commented Dec 14, 2020

[CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first #2237

[CALCITE-4368] TopDownOptTest fails if applying non-substitution rule first #2237

Conversation

chunweilei commented Oct 30, 2020 • edited

danny0405 commented Nov 2, 2020

chunweilei commented Nov 2, 2020

chunweilei commented Nov 2, 2020

vlsi commented Nov 2, 2020

vlsi commented Nov 2, 2020

chunweilei commented Nov 2, 2020

chunweilei commented Nov 2, 2020 • edited

vlsi commented Nov 2, 2020

chunweilei commented Nov 2, 2020

vlsi commented Nov 2, 2020

chunweilei commented Nov 2, 2020 • edited

vlsi commented Nov 2, 2020

chunweilei commented Nov 2, 2020

chunweilei commented Nov 2, 2020 • edited

chunweilei commented Nov 2, 2020

vlsi Nov 2, 2020

Choose a reason for hiding this comment

chunweilei Nov 2, 2020

Choose a reason for hiding this comment

vlsi Nov 2, 2020

Choose a reason for hiding this comment

chunweilei Nov 2, 2020 • edited

Choose a reason for hiding this comment

vlsi commented Nov 2, 2020

chunweilei commented Nov 2, 2020

chunweilei commented Nov 2, 2020

vlsi commented Nov 2, 2020

chunweilei commented Nov 2, 2020

chunweilei commented Nov 4, 2020

chunweilei commented Dec 14, 2020

chunweilei commented Oct 30, 2020 •

edited

chunweilei commented Nov 2, 2020 •

edited

chunweilei commented Nov 2, 2020 •

edited

chunweilei commented Nov 2, 2020 •

edited

chunweilei Nov 2, 2020 •

edited