[
https://issues.apache.org/jira/browse/CALCITE-3118?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Danny Chan resolved CALCITE-3118.
---------------------------------
Resolution: Fixed
Assignee: Danny Chan
Fix Version/s: 1.21.0
Fixed in
[2d52dd3|https://github.com/apache/calcite/commit/2d52dd3f2dc3e6087c034008037e6c47b201c732],
thanks for your PR, [~botong] !
> VolcanoRuleCall should look at RelSubset rather than RelSet when checking
> child ordinal of a parent operand
> -----------------------------------------------------------------------------------------------------------
>
> Key: CALCITE-3118
> URL: https://issues.apache.org/jira/browse/CALCITE-3118
> Project: Calcite
> Issue Type: Bug
> Reporter: Botong Huang
> Assignee: Danny Chan
> Priority: Major
> Labels: pull-request-available
> Fix For: 1.21.0
>
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> In VolcanoRuleCall.matchRecurse(), when ascending (child operand is matched,
> looking for parent operand match), we want to check that the matched parent
> indeed has the previously matched RelNode as a child *with the* *expected
> child ordinal*.
> However, there is a bug in this child ordinal check:
> {noformat}
> 333 if (ascending && operand.childPolicy !=
> RelOptRuleOperandChildPolicy.UNORDERED) {
> 334 // We know that the previous operand was *a* child of its parent,
> 335 // but now check that it is the *correct* child.
> 336 final RelSubset input =
> 337 (RelSubset) rel.getInput(previousOperand.ordinalInParent);
> 338 List<RelNode> inputRels = input.set.getRelsFromAllSubsets();
> 339 if (!inputRels.contains(previous)) {
> 340 continue;
> 341 }
> 342 }
> {noformat}
> We intend to make sure that "previous" is in Subset "input". However line 338
> looked at RelNodes from the entire RelSet, rather than RelNodes only in
> Subset "input". As a result, in some cases, incorrect parent is not skipped
> as expected and is matched incorrectly.
> The unit test case included is a case that triggers this bug, where a second
> child RelNode incorrectly get matched as the first child of the parent
> RelNode.
> --------------------------
> Here's a detailed explanation of the test case that triggers the bug
> We constructed a RelNode structure:
> {noformat}
> PhysBiRel
> / \
> Subset1 Subset2
> | |
> leftPhy rightPhy
> {noformat}
> Where PhysBiRel has two inputs: leftPhy and rightPhy, both are logically
> equivalent, but with different traits (Convention in this test case), and
> thus are in different subsets.
> (Two Rels in two subsets in the same RelSet is a necessary condition to
> trigger this bug. )
> A rule AssertOperandsDifferentRule is constructed as follows:
> {noformat}
> operand(PhysBiRel.class,
> operand(PhysLeafRel.class, any()),
> operand(PhysLeafRel.class, any()))
> {noformat}
> Obviously the correct match would be:
> {noformat}
> PhysBiRel
> / \
> leftPhy rightPhy
> {noformat}
> However, with the bug, another match is also returned:
> {noformat}
> PhysBiRel
> / \
> rightPhy rightPhy
> {noformat}
> *This is wrong because rightPhy is not PhysBiRel's first input at all, and
> should not match as parent operand's first child.*
> ----------------------------
> Here's how the incorrect match occurs.
> 1. When rightPhy of class PhysLeafRel is registered, we attempt to match it
> to both the left and right child operands of AssertOperandsDifferentRule
> above. This is expected.
> 2. When matched to the right child operand, it eventually leads to the
> correct match result above.
> 3. When matched to the left child operand, it should have skipped/failed
> matching the parent operand to PhysBiRel because rightPhy is *NOT*
> PhysBiRel's first input. But because of the bug, the match succeeded. After
> parent is matched, then it attempt to match the right child operand, and
> again matched the rightPhy. As a result, both child operand end up matching
> the same RelNode rightPhy.
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
