[CALCITE-2018] Queries failed with AssertionError: rel has lower cost… #552

vvysotskyi · 2017-10-23T14:34:18Z

… than best cost of subset.

Replaced Map usage by Table for query metadata caching. It allows finding cached metadata query for concrete RelNode instance more easily.
Made changes to use single RelMetadataQuery instance during planning between the first call of RelOptCluster.getMetadataQuery() and RelOptCluster.invalidateMetadataQuery() call.
Cached metadata values are removed for current RelSubset and all parent RelNodes when RelSubset.propagateCostImprovements() is called and best rel node was changed.
Made changes in RelSet.mergeWith() method to call RelSubset.propagateCostImprovements() when resulting best is known instead of just assigning RelSubset.best.

… than best cost of subset. (Second attempt) (cherry picked from commit 1c795d7)

vlsi · 2019-01-09T20:32:44Z

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

      if (otherSubset.bestCost.isLt(subset.bestCost)) {
-        subset.bestCost = otherSubset.bestCost;
-        subset.best = otherSubset.best;
+        changedSubsets.put(subset, otherSubset.best);


Should something like max(changedSubsets.get(subset), otherSubset.best) be used here?

Good assumption, but when registering new rel nodes into this RelSet from otherSubset, the best cost for subset may be changed, and therefore otherSubset.best should be passed, since it is better according to the check above. Also, otherSubset is not changed and therefore there is no need to try to update its best cost.

… than best cost of subset. (Inspired from pr: apache#552 which is committed by vvysotskyi). Summary: 1.Replaced Map usage by Table for query metadata caching. It allows finding cached metadata query for concrete RelNode instance more easily. 2. Cached metadata values are removed for current RelSubset and all parent RelNodes when RelSubset.propagateCostImprovements() is called and best rel node was changed. 3. Made changes in RelSet.mergeWith() method to call RelSubset.propagateCostImprovements() when resulting best is known instead of just assigning RelSubset.best. Note: 1. Does not import following points from pr because it will increase the lifetime of RelMetadataQuery. Made changes to use single RelMetadataQuery instance during planning between the first call of RelOptCluster.getMetadataQuery() and RelOptCluster.invalidateMetadataQuery() call. Test Plan: NO Reviewers: 鲁尼, 晓令, 玉兆 Reviewed By: 晓令 Subscribers: P577102 Differential Revision: https://aone.alibaba-inc.com/code/D642962

danny0405 · 2019-03-03T04:38:18Z

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

+      RelSubset relSubset = subsetBestPair.getKey();
+      relSubset.propagateCostImprovements(
+          planner, mq, subsetBestPair.getValue(),
+          activeSet);


Okey, this is definitely needed. We should reCompute the best rel based on the new cost instead of replace it directly with the best rel in the merged RelSubSet.

Agree, this is one of the key changes in this PR.

What's the reason of recomputing otherSubset.best? Do you think its input can be changed at this point? Can you please give a scenario? Thanks.

It is required for the case when this RelSet contains RelSubset which matches to the trait sets of otherSubset, so best may be changed (otherSubset.bestCost.isLt(subset.bestCost) check) and therefore parent rel nodes should be updated.

Thanks. Now I see why. It looks to me that if we move these codes to be after line 363 (renaming parentRel), then we won't need line 373 to line 378. What do you think?

xndai · 2019-09-09T06:36:21Z

Also I notice under debugger that RelNode tree can have more than one RelOptCluster. I haven't dig into that. But if this is the case, your #2 bullet point in your change description may not be true.

zabetak · 2019-09-09T09:52:00Z

Hi @xndai if you have two clusters in the same RelNode tree then it sounds like a bug. There are various checks in the VolcanoPlanner to avoid this situation.

xndai · 2019-09-10T00:08:52Z

@zabetak @vvysotskyi I look closer and find two tests cases in JdbcTest where I see rel nodes have different RelOptCluster instances - testLateralJoin and testExpandViewWithLateralJoin. It looks like during parsing when there's view table and ViewTable.expandView() would create a new SqlToRelConverter instance which then would use a different RelOptCluster instance for rel nodes it creates.

Is this expected?

zabetak · 2019-09-12T14:06:14Z

@xndai thanks for looking into this further. I kind of understand why it happens but I think it should not.

xndai · 2019-09-15T21:41:44Z

I would also suggest we turn on isValid() check by default in UT (instead of doing that only under Trace logger level). We would expect all these kinds of issues are gone with this fix (and 2116). Also to prevent any future regressions.

xndai · 2019-09-24T01:23:03Z

Based on this change, I made a couple of modifications, now the isValid() check passes for all unit tests. You can find at my changes at - https://github.com/xndai/calcite/tree/2018

The key thing I add is that when a RelNode is renamed and it is subset's best RelNode, we propagate change improvements since the subset's best cost could be changed. I also turn on isValid() check under assert.

Also with this change, we run into some CyclicMetadataException when accessing meta. Right now I just catch and ignore, similar to how it is handled in other parts of the code. But in the long run, we will need to figure out better way to handle cyclic.

@vvysotskyi @zabetak

danny0405 · 2019-09-28T02:22:04Z

Thanks @xndai for the additional patch, the CyclicMetadataException already says that exception is thrown for signaling purposes, so the stack trace is not really important. Also i rechecked the code and make sure that the fix of @vvysotskyi is the right way to go, we should consider:

Propagate the cost when 2 RelSet are merged.
Clear the cached metadata for parents when propagating.

hsyuan · 2019-09-28T02:31:41Z

But the stack trace is misleading if we reuse the exception instance. On the other hand, how much time will be wasted if we construct new CyclicMetadataException instance without reusing the singleton? I don't think it is significant.

xndai · 2019-09-29T03:16:19Z

I agree with Haisheng. The exception needs to be fixed. Currently we could get a wrong call stack info when reusing CyclicMetadataException. This is really confusing and creates a lot of pains during debugging.

… than best cost of subset

The change completes the fix of CALCITE-2018. Below are a few key additions - 1. When a Rel is renamed and it is subset's best RelNode, we need to propagate change improvement since the subset's best cost could be changed. 2. Add codes to handle CyclicMetadataException. Right now we just ignore it. We will revisit once we have figured out a better way to handle CyclicMetadataException. 3. Enable isValid() check under assert. Also check validation after RelSet merge.

xndai · 2019-10-29T17:45:35Z

LGTM

danny0405

+1

hsyuan

+1

hsyuan · 2019-10-30T04:32:24Z

@danny0405 You didn't squash commits?

danny0405 · 2019-10-30T04:40:33Z

Yes, i want to keep the credit of every committer.

hsyuan · 2019-10-30T04:42:52Z

You don't need to. There is a way to keep multiple authors, like 955d4ea

danny0405 · 2019-10-30T04:47:16Z

Thanks, but i want to follow the CALCITE-1935 style because this fix lasts so long time and so many discussions, split it into multiple commits make the fix more clear.

… than best cost of subset * Replaced Map usage by Table for query metadata caching. It allows finding cached metadata query for concrete RelNode instance more easily. * Made changes to use single RelMetadataQuery instance during planning between the first call of RelOptCluster.getMetadataQuery() and RelOptCluster.invalidateMetadataQuery() call. * Cached metadata values are removed for current RelSubset and all parent RelNodes when RelSubset.propagateCostImprovements() is called and best rel node was changed. * Made changes in RelSet.mergeWith() method to call RelSubset.propagateCostImprovements() when resulting best is known instead of just assigning RelSubset.best. close apache#552

… than best cost of subset * Replaced Map usage by Table for query metadata caching. It allows finding cached metadata query for concrete RelNode instance more easily. * Made changes to use single RelMetadataQuery instance during planning between the first call of RelOptCluster.getMetadataQuery() and RelOptCluster.invalidateMetadataQuery() call. * Cached metadata values are removed for current RelSubset and all parent RelNodes when RelSubset.propagateCostImprovements() is called and best rel node was changed. * Made changes in RelSet.mergeWith() method to call RelSubset.propagateCostImprovements() when resulting best is known instead of just assigning RelSubset.best. close apache#552 Change-Id: I3590638395bde98699980b73a891aef441ec7167

asfgit force-pushed the master branch from a147d1a to 20eee63 Compare November 9, 2017 15:34

vvysotskyi force-pushed the CALCITE-2018 branch from 7ebf238 to f733d24 Compare December 18, 2017 09:24

vvysotskyi force-pushed the CALCITE-2018 branch from f733d24 to 73cb945 Compare December 28, 2017 12:39

vvysotskyi referenced this pull request in KulykRoman/incubator-calcite Jan 12, 2018

[CALCITE-2018] Queries failed with AssertionError: rel has lower cost…

af04ef4

… than best cost of subset. (Second attempt) (cherry picked from commit 1c795d7)

asfgit force-pushed the master branch from 5b3d3d3 to be2fe5f Compare January 19, 2018 18:04

asfgit force-pushed the master branch from 5acf84f to 0e13a8a Compare February 17, 2018 03:36

asfgit force-pushed the master branch from 705d388 to 0204f28 Compare July 9, 2018 16:16

asfgit force-pushed the master branch from 87c4b73 to a5f09f5 Compare September 24, 2018 04:29

vlsi mentioned this pull request Jan 8, 2019

[CALCITE-2204] Use BFS for propagating cost #643

Closed

vlsi reviewed Jan 9, 2019

View reviewed changes

danny0405 reviewed Mar 3, 2019

View reviewed changes

vvysotskyi force-pushed the CALCITE-2018 branch 2 times, most recently from 78b4def to dfa8317 Compare April 16, 2019 10:30

julianhyde force-pushed the master branch from 2c19098 to f3758a6 Compare September 16, 2019 17:45

vvysotskyi and others added 3 commits October 29, 2019 11:13

[CALCITE-2018] Queries failed with AssertionError: rel has lower cost…

e087479

… than best cost of subset

[CALCITE-2018] Use cluster at the same place where planner was

3920bc2

vvysotskyi force-pushed the CALCITE-2018 branch from dfa8317 to 3920bc2 Compare October 29, 2019 09:28

danny0405 approved these changes Oct 30, 2019

View reviewed changes

danny0405 added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Oct 30, 2019

hsyuan approved these changes Oct 30, 2019

View reviewed changes

danny0405 closed this in e0d869a Oct 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-2018] Queries failed with AssertionError: rel has lower cost… #552

[CALCITE-2018] Queries failed with AssertionError: rel has lower cost… #552

vvysotskyi commented Oct 23, 2017 •

edited

Loading

vlsi Jan 9, 2019

vvysotskyi Apr 16, 2019

danny0405 Mar 3, 2019 •

edited

Loading

vvysotskyi Apr 16, 2019

xndai Aug 23, 2019

vvysotskyi Sep 7, 2019

xndai Sep 9, 2019

xndai commented Sep 9, 2019

zabetak commented Sep 9, 2019

xndai commented Sep 10, 2019

zabetak commented Sep 12, 2019

xndai commented Sep 15, 2019

xndai commented Sep 24, 2019

danny0405 commented Sep 28, 2019

hsyuan commented Sep 28, 2019

xndai commented Sep 29, 2019

xndai commented Oct 29, 2019

danny0405 left a comment

hsyuan left a comment

hsyuan commented Oct 30, 2019

danny0405 commented Oct 30, 2019

hsyuan commented Oct 30, 2019

danny0405 commented Oct 30, 2019

[CALCITE-2018] Queries failed with AssertionError: rel has lower cost… #552

[CALCITE-2018] Queries failed with AssertionError: rel has lower cost… #552

Conversation

vvysotskyi commented Oct 23, 2017 • edited Loading

vlsi Jan 9, 2019

Choose a reason for hiding this comment

vvysotskyi Apr 16, 2019

Choose a reason for hiding this comment

danny0405 Mar 3, 2019 • edited Loading

Choose a reason for hiding this comment

vvysotskyi Apr 16, 2019

Choose a reason for hiding this comment

xndai Aug 23, 2019

Choose a reason for hiding this comment

vvysotskyi Sep 7, 2019

Choose a reason for hiding this comment

xndai Sep 9, 2019

Choose a reason for hiding this comment

xndai commented Sep 9, 2019

zabetak commented Sep 9, 2019

xndai commented Sep 10, 2019

zabetak commented Sep 12, 2019

xndai commented Sep 15, 2019

xndai commented Sep 24, 2019

danny0405 commented Sep 28, 2019

hsyuan commented Sep 28, 2019

xndai commented Sep 29, 2019

xndai commented Oct 29, 2019

danny0405 left a comment

Choose a reason for hiding this comment

hsyuan left a comment

Choose a reason for hiding this comment

hsyuan commented Oct 30, 2019

danny0405 commented Oct 30, 2019

hsyuan commented Oct 30, 2019

danny0405 commented Oct 30, 2019

vvysotskyi commented Oct 23, 2017 •

edited

Loading

danny0405 Mar 3, 2019 •

edited

Loading