[
https://issues.apache.org/jira/browse/CALCITE-3118?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16861715#comment-16861715
]
Danny Chan commented on CALCITE-3118:
-------------------------------------
Okey, finally i believe this is a bug
For this case, it happens with VolcanoRuleCall#getOperand0().solveOrder as [1,
0, 2],
for a tree with asending match mode like:
{code:java}
Birel(0)
/ \
left(1) right(2){code}
The VolcanoRuleCall firstly match right(2); then it start to match Birel(0),
obviously it is the successor of right(2), but when we do check
{code:java}
final RelSubset input =
(RelSubset) rel.getInput(previousOperand.ordinalInParent);
List<RelNode> inputRels = input.set.getRelsFromAllSubsets();
if (!inputRels.contains(previous)) {
continue;
}{code}
It got passed only because right(2) is the child of Birel(0), but as to the
sloveOrder, right(2) expects to be the second child of Birel(0), but with this
successfull match, it is the first child of Birel(0).
> VolcanoRuleCall match parent child ordinal not properly checked
> ---------------------------------------------------------------
>
> Key: CALCITE-3118
> URL: https://issues.apache.org/jira/browse/CALCITE-3118
> Project: Calcite
> Issue Type: Bug
> Reporter: Botong Huang
> Priority: Major
> Labels: pull-request-available
> Time Spent: 1h 20m
> 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.3#76005)
