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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 < >>>> [email protected]> έγραψε: >>>> >>>>> 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 >>> >> >
