You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Vasiliki Kalavri <va...@gmail.com> on 2016/03/07 16:10:46 UTC
LogicalUnion check in JoinUnionTransposeRule
Hello everyone,
the JoinUnionTransposeRule is fired if one of the operands is a Union, but
inside the onMatch() method, the code checks if one of the operands is an
instance of LogicalUnion [1]*.*
In the Apache Flink SQL implementation, we have created a logical operator
FlinkUnion for the translation of union. This operator extends Union and
thus when the JoinUnionTransposeRule matches, it throws a
ClassCastException. To avoid this, we have copied the
JoinUnionTransposeRule over to Flink and changed the check in onMatch()
method to look for an instance of Union instead.
Obviously, this is not a good choice, since the rule code should not have
to be maintained on the Flink side. We are wondering whether this check is
a bug or whether we have misunderstood the way we are supposed to use this
rule.
Thank you,
-Vasia.
[1]:
https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/JoinUnionTransposeRule.java#L61
Re: LogicalUnion check in JoinUnionTransposeRule
Posted by Julian Hyde <jh...@apache.org>.
Further to what Jesus already said: there are mechanisms to allow rules to be re-used across multiple engines. Copy-pasting the rule into Flink is a a good short-term measure but not good long-term. (Rules are complex pieces of code, and contain unknown bugs. Your copy of the rule will not receive fixes when we discover and fix those bugs.)
We try to make rules re-usable by parameterizing them with the RelNode classes they should apply to, and by using RelBuilder or RelNode.copy to create RelNodes of the appropriate sub-types. But sometimes we forget, and we need bugs like CALCITE-856 to remind us. Once your bug is fixed, you should be able to create a trivial sub-class of JoinUnionTransposeRule that has the desired behavior for Flink.
Julian
> On Mar 7, 2016, at 11:39 AM, Vasiliki Kalavri <va...@gmail.com> wrote:
>
> Thanks Jesús! I'll open a JIRA and a corresponding PR.
>
> -Vasia.
>
> On 7 March 2016 at 18:25, Jesus Camacho Rodriguez <
> jcamachorodriguez@hortonworks.com> wrote:
>
>> Hi Vasia,
>>
>> You did the right thing on the Flink side. This has happened with multiple
>> rules previously; they were originally implemented using the Logical
>> convention and till we do not use them externally, we do not hit this kind
>> of issues. As we make Calcite more extensible, the rules are updated.
>>
>> Could you file a bug as in [1] and contribute the fix? Otherwise, you can
>> assign it to me, as you prefer.
>>
>> Thanks,
>> Jesús
>>
>> [1] https://issues.apache.org/jira/browse/CALCITE-856
>>
>>
>>
>>
>>
>> On 3/7/16, 4:10 PM, "Vasiliki Kalavri" <va...@gmail.com> wrote:
>>
>>> Hello everyone,
>>>
>>> the JoinUnionTransposeRule is fired if one of the operands is a Union, but
>>> inside the onMatch() method, the code checks if one of the operands is an
>>> instance of LogicalUnion [1]*.*
>>>
>>> In the Apache Flink SQL implementation, we have created a logical operator
>>> FlinkUnion for the translation of union. This operator extends Union and
>>> thus when the JoinUnionTransposeRule matches, it throws a
>>> ClassCastException. To avoid this, we have copied the
>>> JoinUnionTransposeRule over to Flink and changed the check in onMatch()
>>> method to look for an instance of Union instead.
>>>
>>> Obviously, this is not a good choice, since the rule code should not have
>>> to be maintained on the Flink side. We are wondering whether this check is
>>> a bug or whether we have misunderstood the way we are supposed to use this
>>> rule.
>>>
>>> Thank you,
>>> -Vasia.
>>>
>>> [1]:
>>>
>> https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/JoinUnionTransposeRule.java#L61
>>
Re: LogicalUnion check in JoinUnionTransposeRule
Posted by Vasiliki Kalavri <va...@gmail.com>.
Thanks Jesús! I'll open a JIRA and a corresponding PR.
-Vasia.
On 7 March 2016 at 18:25, Jesus Camacho Rodriguez <
jcamachorodriguez@hortonworks.com> wrote:
> Hi Vasia,
>
> You did the right thing on the Flink side. This has happened with multiple
> rules previously; they were originally implemented using the Logical
> convention and till we do not use them externally, we do not hit this kind
> of issues. As we make Calcite more extensible, the rules are updated.
>
> Could you file a bug as in [1] and contribute the fix? Otherwise, you can
> assign it to me, as you prefer.
>
> Thanks,
> Jesús
>
> [1] https://issues.apache.org/jira/browse/CALCITE-856
>
>
>
>
>
> On 3/7/16, 4:10 PM, "Vasiliki Kalavri" <va...@gmail.com> wrote:
>
> >Hello everyone,
> >
> >the JoinUnionTransposeRule is fired if one of the operands is a Union, but
> >inside the onMatch() method, the code checks if one of the operands is an
> >instance of LogicalUnion [1]*.*
> >
> >In the Apache Flink SQL implementation, we have created a logical operator
> >FlinkUnion for the translation of union. This operator extends Union and
> >thus when the JoinUnionTransposeRule matches, it throws a
> >ClassCastException. To avoid this, we have copied the
> >JoinUnionTransposeRule over to Flink and changed the check in onMatch()
> >method to look for an instance of Union instead.
> >
> >Obviously, this is not a good choice, since the rule code should not have
> >to be maintained on the Flink side. We are wondering whether this check is
> >a bug or whether we have misunderstood the way we are supposed to use this
> >rule.
> >
> >Thank you,
> >-Vasia.
> >
> >[1]:
> >
> https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/JoinUnionTransposeRule.java#L61
>
Re: LogicalUnion check in JoinUnionTransposeRule
Posted by Jesus Camacho Rodriguez <jc...@hortonworks.com>.
Hi Vasia,
You did the right thing on the Flink side. This has happened with multiple rules previously; they were originally implemented using the Logical convention and till we do not use them externally, we do not hit this kind of issues. As we make Calcite more extensible, the rules are updated.
Could you file a bug as in [1] and contribute the fix? Otherwise, you can assign it to me, as you prefer.
Thanks,
Jesús
[1] https://issues.apache.org/jira/browse/CALCITE-856
On 3/7/16, 4:10 PM, "Vasiliki Kalavri" <va...@gmail.com> wrote:
>Hello everyone,
>
>the JoinUnionTransposeRule is fired if one of the operands is a Union, but
>inside the onMatch() method, the code checks if one of the operands is an
>instance of LogicalUnion [1]*.*
>
>In the Apache Flink SQL implementation, we have created a logical operator
>FlinkUnion for the translation of union. This operator extends Union and
>thus when the JoinUnionTransposeRule matches, it throws a
>ClassCastException. To avoid this, we have copied the
>JoinUnionTransposeRule over to Flink and changed the check in onMatch()
>method to look for an instance of Union instead.
>
>Obviously, this is not a good choice, since the rule code should not have
>to be maintained on the Flink side. We are wondering whether this check is
>a bug or whether we have misunderstood the way we are supposed to use this
>rule.
>
>Thank you,
>-Vasia.
>
>[1]:
>https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/rel/rules/JoinUnionTransposeRule.java#L61