You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Piotr Nowojski <pi...@da-platform.com> on 2019/01/31 16:39:43 UTC
Intention behind RelNode.accept(RexShuttle)
Hi!
We are adding a new custom RelNode, that behaves a little bit like Aggregate node and we are wondering how we should implement
RelNode#accept(org.apache.calcite.rex.RexShuttle)
Our node has a similar feature as Aggregate node, that it’s keying by the incoming data. At first I thought that this #accept() method should be implemented by applying the RexShuttle to all of the expressions that node is using internally (in our case key fields “expressions”/“references"), like it’s done in Join, Sort, Filter, …. But then I noticed that this method is not implemented for Aggregate node. So can we leave this method with the default implementation `return this;` as Aggregate node does it?
Thanks, Piotr Nowojski
Re: Intention behind RelNode.accept(RexShuttle)
Posted by Julian Hyde <jh...@apache.org>.
I agree, a stronger contract would be better. We would gladly accept improved documentation and tests. Once those are in place, we will live by the contract (or discuss why we want to change the contract).
It’s worth noting in the contract (because people might find it surprising) that Sort does have RexNode children, and is therefore affected by the shuttle, but Aggregate does not.
I notice that the RelNode.accept(RexShuttle) was introduced in https://issues.apache.org/jira/browse/CALCITE-479 <https://issues.apache.org/jira/browse/CALCITE-479>. If you read that case you can see what we intended; I think we still live up to that intent, but we could of course always use more tests and documentation.
Julian
> On Feb 5, 2019, at 12:45 AM, Piotr Nowojski <pi...@da-platform.com> wrote:
>
> Thanks again for the answer, we will work with that.
>
> One comment from my side: this lack of the contract is not ideal. I could imagine `accept(RexShuttle)` being used in the future for variety of things. For example if one day Calcite starts using `accept(RexShuttle)` for gathering used field references for column pruning, our code (that uses Calcite) can brake silently.
>
> Piotrek
>
>> On 4 Feb 2019, at 20:04, Julian Hyde <jh...@apache.org> wrote:
>>
>> RexShuttle does not have a watertight specification. It does what it needs to do for the cases that use it. Generally, if the sub-elements of a RelNode are sub-types of RexNode, RexShuttle processes them, otherwise it ignores them. But pragmatically, as long as what you do doesn’t break the tests, it’s OK.
>>
>>> On Feb 4, 2019, at 1:05 AM, Piotr Nowojski <pi...@da-platform.com> wrote:
>>>
>>> Hi Stamatis,
>>>
>>> Thank you for the answer! I have some questions to clarify that.
>>>
>>> 1. What if my new node doesn’t strictly have a `RexNode`, but it’s referring to the inputs indirectly, like via `String fieldName` instead of using RexInputRef?
>>> 2. Arguably this is the similar thing what `Aggregate` node is doing. Both `List<ImmutableBitSet> groupSets` and `List<AggregateCall> aggCalls` (in `Aggregate`) are referring to the operator/node inputs using indexes.
>>>
>>> Piotrek
>>>
>>>> On 2 Feb 2019, at 15:37, Stamatis Zampetakis <za...@gmail.com> wrote:
>>>>
>>>> Hi Piotr,
>>>>
>>>> Aggregate is not doing anything in #accept(RexShuttle) since it does not
>>>> contain row expressions (RexNode). If the node you introduce uses row
>>>> expressions then it makes sense to apply the shuttle to every expression.
>>>>
>>>> Best,
>>>> Stamatis
>>>>
>>>> Στις Πέμ, 31 Ιαν 2019 στις 5:39 μ.μ., ο/η Piotr Nowojski <
>>>> piotr@da-platform.com> έγραψε:
>>>>
>>>>> Hi!
>>>>>
>>>>> We are adding a new custom RelNode, that behaves a little bit like
>>>>> Aggregate node and we are wondering how we should implement
>>>>>
>>>>> RelNode#accept(org.apache.calcite.rex.RexShuttle)
>>>>>
>>>>> Our node has a similar feature as Aggregate node, that it’s keying by the
>>>>> incoming data. At first I thought that this #accept() method should be
>>>>> implemented by applying the RexShuttle to all of the expressions that node
>>>>> is using internally (in our case key fields “expressions”/“references"),
>>>>> like it’s done in Join, Sort, Filter, …. But then I noticed that this
>>>>> method is not implemented for Aggregate node. So can we leave this method
>>>>> with the default implementation `return this;` as Aggregate node does it?
>>>>>
>>>>> Thanks, Piotr Nowojski
>>>
>>
>
Re: Intention behind RelNode.accept(RexShuttle)
Posted by Piotr Nowojski <pi...@da-platform.com>.
Thanks again for the answer, we will work with that.
One comment from my side: this lack of the contract is not ideal. I could imagine `accept(RexShuttle)` being used in the future for variety of things. For example if one day Calcite starts using `accept(RexShuttle)` for gathering used field references for column pruning, our code (that uses Calcite) can brake silently.
Piotrek
> On 4 Feb 2019, at 20:04, Julian Hyde <jh...@apache.org> wrote:
>
> RexShuttle does not have a watertight specification. It does what it needs to do for the cases that use it. Generally, if the sub-elements of a RelNode are sub-types of RexNode, RexShuttle processes them, otherwise it ignores them. But pragmatically, as long as what you do doesn’t break the tests, it’s OK.
>
>> On Feb 4, 2019, at 1:05 AM, Piotr Nowojski <pi...@da-platform.com> wrote:
>>
>> Hi Stamatis,
>>
>> Thank you for the answer! I have some questions to clarify that.
>>
>> 1. What if my new node doesn’t strictly have a `RexNode`, but it’s referring to the inputs indirectly, like via `String fieldName` instead of using RexInputRef?
>> 2. Arguably this is the similar thing what `Aggregate` node is doing. Both `List<ImmutableBitSet> groupSets` and `List<AggregateCall> aggCalls` (in `Aggregate`) are referring to the operator/node inputs using indexes.
>>
>> Piotrek
>>
>>> On 2 Feb 2019, at 15:37, Stamatis Zampetakis <za...@gmail.com> wrote:
>>>
>>> Hi Piotr,
>>>
>>> Aggregate is not doing anything in #accept(RexShuttle) since it does not
>>> contain row expressions (RexNode). If the node you introduce uses row
>>> expressions then it makes sense to apply the shuttle to every expression.
>>>
>>> Best,
>>> Stamatis
>>>
>>> Στις Πέμ, 31 Ιαν 2019 στις 5:39 μ.μ., ο/η Piotr Nowojski <
>>> piotr@da-platform.com> έγραψε:
>>>
>>>> Hi!
>>>>
>>>> We are adding a new custom RelNode, that behaves a little bit like
>>>> Aggregate node and we are wondering how we should implement
>>>>
>>>> RelNode#accept(org.apache.calcite.rex.RexShuttle)
>>>>
>>>> Our node has a similar feature as Aggregate node, that it’s keying by the
>>>> incoming data. At first I thought that this #accept() method should be
>>>> implemented by applying the RexShuttle to all of the expressions that node
>>>> is using internally (in our case key fields “expressions”/“references"),
>>>> like it’s done in Join, Sort, Filter, …. But then I noticed that this
>>>> method is not implemented for Aggregate node. So can we leave this method
>>>> with the default implementation `return this;` as Aggregate node does it?
>>>>
>>>> Thanks, Piotr Nowojski
>>
>
Re: Intention behind RelNode.accept(RexShuttle)
Posted by Julian Hyde <jh...@apache.org>.
RexShuttle does not have a watertight specification. It does what it needs to do for the cases that use it. Generally, if the sub-elements of a RelNode are sub-types of RexNode, RexShuttle processes them, otherwise it ignores them. But pragmatically, as long as what you do doesn’t break the tests, it’s OK.
> On Feb 4, 2019, at 1:05 AM, Piotr Nowojski <pi...@da-platform.com> wrote:
>
> Hi Stamatis,
>
> Thank you for the answer! I have some questions to clarify that.
>
> 1. What if my new node doesn’t strictly have a `RexNode`, but it’s referring to the inputs indirectly, like via `String fieldName` instead of using RexInputRef?
> 2. Arguably this is the similar thing what `Aggregate` node is doing. Both `List<ImmutableBitSet> groupSets` and `List<AggregateCall> aggCalls` (in `Aggregate`) are referring to the operator/node inputs using indexes.
>
> Piotrek
>
>> On 2 Feb 2019, at 15:37, Stamatis Zampetakis <za...@gmail.com> wrote:
>>
>> Hi Piotr,
>>
>> Aggregate is not doing anything in #accept(RexShuttle) since it does not
>> contain row expressions (RexNode). If the node you introduce uses row
>> expressions then it makes sense to apply the shuttle to every expression.
>>
>> Best,
>> Stamatis
>>
>> Στις Πέμ, 31 Ιαν 2019 στις 5:39 μ.μ., ο/η Piotr Nowojski <
>> piotr@da-platform.com> έγραψε:
>>
>>> Hi!
>>>
>>> We are adding a new custom RelNode, that behaves a little bit like
>>> Aggregate node and we are wondering how we should implement
>>>
>>> RelNode#accept(org.apache.calcite.rex.RexShuttle)
>>>
>>> Our node has a similar feature as Aggregate node, that it’s keying by the
>>> incoming data. At first I thought that this #accept() method should be
>>> implemented by applying the RexShuttle to all of the expressions that node
>>> is using internally (in our case key fields “expressions”/“references"),
>>> like it’s done in Join, Sort, Filter, …. But then I noticed that this
>>> method is not implemented for Aggregate node. So can we leave this method
>>> with the default implementation `return this;` as Aggregate node does it?
>>>
>>> Thanks, Piotr Nowojski
>
Re: Intention behind RelNode.accept(RexShuttle)
Posted by Piotr Nowojski <pi...@da-platform.com>.
Hi Stamatis,
Thank you for the answer! I have some questions to clarify that.
1. What if my new node doesn’t strictly have a `RexNode`, but it’s referring to the inputs indirectly, like via `String fieldName` instead of using RexInputRef?
2. Arguably this is the similar thing what `Aggregate` node is doing. Both `List<ImmutableBitSet> groupSets` and `List<AggregateCall> aggCalls` (in `Aggregate`) are referring to the operator/node inputs using indexes.
Piotrek
> On 2 Feb 2019, at 15:37, Stamatis Zampetakis <za...@gmail.com> wrote:
>
> Hi Piotr,
>
> Aggregate is not doing anything in #accept(RexShuttle) since it does not
> contain row expressions (RexNode). If the node you introduce uses row
> expressions then it makes sense to apply the shuttle to every expression.
>
> Best,
> Stamatis
>
> Στις Πέμ, 31 Ιαν 2019 στις 5:39 μ.μ., ο/η Piotr Nowojski <
> piotr@da-platform.com> έγραψε:
>
>> Hi!
>>
>> We are adding a new custom RelNode, that behaves a little bit like
>> Aggregate node and we are wondering how we should implement
>>
>> RelNode#accept(org.apache.calcite.rex.RexShuttle)
>>
>> Our node has a similar feature as Aggregate node, that it’s keying by the
>> incoming data. At first I thought that this #accept() method should be
>> implemented by applying the RexShuttle to all of the expressions that node
>> is using internally (in our case key fields “expressions”/“references"),
>> like it’s done in Join, Sort, Filter, …. But then I noticed that this
>> method is not implemented for Aggregate node. So can we leave this method
>> with the default implementation `return this;` as Aggregate node does it?
>>
>> Thanks, Piotr Nowojski
Re: Intention behind RelNode.accept(RexShuttle)
Posted by Stamatis Zampetakis <za...@gmail.com>.
Hi Piotr,
Aggregate is not doing anything in #accept(RexShuttle) since it does not
contain row expressions (RexNode). If the node you introduce uses row
expressions then it makes sense to apply the shuttle to every expression.
Best,
Stamatis
Στις Πέμ, 31 Ιαν 2019 στις 5:39 μ.μ., ο/η Piotr Nowojski <
piotr@da-platform.com> έγραψε:
> Hi!
>
> We are adding a new custom RelNode, that behaves a little bit like
> Aggregate node and we are wondering how we should implement
>
> RelNode#accept(org.apache.calcite.rex.RexShuttle)
>
> Our node has a similar feature as Aggregate node, that it’s keying by the
> incoming data. At first I thought that this #accept() method should be
> implemented by applying the RexShuttle to all of the expressions that node
> is using internally (in our case key fields “expressions”/“references"),
> like it’s done in Join, Sort, Filter, …. But then I noticed that this
> method is not implemented for Aggregate node. So can we leave this method
> with the default implementation `return this;` as Aggregate node does it?
>
> Thanks, Piotr Nowojski