[
https://issues.apache.org/jira/browse/CALCITE-3118?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16859312#comment-16859312
]
Botong Huang edited comment on CALCITE-3118 at 6/8/19 9:29 PM:
---------------------------------------------------------------
{quote}
Firstly you construct a RelNode tree like what the pattern describes, then you
register that the `leftPhy` and `rightPhy` are equivalent. It's not that
surprising you got a tree like:
PhysBiRel
/ \
rightPhy rightPhy
This tree also satisfied AssertOperandsDifferentRule patttern.
{quote}
leftPhy and rightPhy are indeed logically equivalent, but they have different
traits (Convention in this test case), and thus are in different subsets.
The actual Rel structure is:
{noformat}
PhysBiRel
/ \
Subset1 Subset2
| |
leftPhy rightPhy
{noformat}
Two Rels in two subsets in the same RelSet is a necessary condition to trigger
this bug.
The reason that I think rightPhy should not match the left child operand is
because of rightPhy's child ordinal in parent. When AssertOperandsDifferentRule
says
{noformat}
operand(PhysBiRel.class,
operand(PhysLeafRel.class, any()),
operand(PhysLeafRel.class, any()))
{noformat}
it implies RelOptRuleOperandChildPolicy.SOME, whose javadoc says "Signifies
that the operand's children must precisely match its child operands, in order."
rightPhy is PhysBiRel's second input, and it should not match as the first
child of the parent operand. It violates the "precisely match in order" part.
In fact, rightPhy is not PhysBiRel's first input at all. I really don't think
this is a valid match.
Enforcing child ordinal is exactly what line 334-341 in VolcanoRuleCall is
trying to do, as the code comment says:
{noformat}
// We know that the previous operand was *a* child of its parent,
// but now check that it is the *correct* child.
{noformat}
Except that in this unit test's case, the code failed to detect and skip
(continue at line 340) this wrong match because it looked at all Rels in the
RelSet instead of looking at the Rels in the Subset only. This is why I insist
this is a bug. Hopefully it is clearer now.
was (Author: botong):
{quote}
Firstly you construct a RelNode tree like what the pattern describes, then you
register that the `leftPhy` and `rightPhy` are equivalent. It's not that
surprising you got a tree like:
PhysBiRel
/ \
rightPhy rightPhy
This tree also satisfied AssertOperandsDifferentRule patttern.
{quote}
leftPhy and rightPhy are indeed logically equivalent, but they have different
traits (Convention in this test case), and thus are in different subsets.
The actual Rel structure is:
{noformat}
PhysBiRel
/ \
Subset1 Subset2
| |
leftPhy rightPhy
{noformat}
Two Rels in two subsets in the same RelSet is a necessary condition to trigger
this bug.
The reason that I think rightPhy should not match the left child operand is
because of rightPhy's child ordinal in parent. When AssertOperandsDifferentRule
says
{noformat}
operand(PhysBiRel.class,
operand(PhysLeafRel.class, any()),
operand(PhysLeafRel.class, any()))
{noformat}
it implies RelOptRuleOperandChildPolicy.SOME, whose javadoc says "Signifies
that the operand's children must precisely match its child operands, in order."
rightPhy is PhysBiRel's second input, and it should not match as the first
child of the parent operand. It violates the "precisely match in order" part.
Enforcing child ordinal is exactly what line 334-341 in VolcanoRuleCall is
trying to do, as the code comment says:
{noformat}
// We know that the previous operand was *a* child of its parent,
// but now check that it is the *correct* child.
{noformat}
Except that in this unit test's case, the code failed to detect and skip
(continue at line 340) this wrong match because it looked at all Rels in the
RelSet instead of looking at the Rels in the Subset only. This is why I insist
this is a bug. Hopefully it is clearer now.
> 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 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 child RelNode as a child with the expected
> ordinal. However, there is a bug in this check. As a result, some incorrect
> parent is not skipped as expected and matched incorrectly. See unit test
> included in PR for a case that triggers this bug, where a second child
> RelNode get matched as first child of the parent RelNode.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
