RelShuttle vs RexShuttle

[Vladimir Sitnikov <si...@gmail.com>]

Hi,

I need a kind of visitor that will be able to rewrite part of relation
tree including rex expressions.
RexShuttle does a good job when rewriting Rex expressions alone (e.g.
calulations, casts, etc).

RelShuttle allows to visit Rel tree (e.g. Project/Filter/etc), however
it does not allow to change expressions inside the tree.

Here's my question: does anybody object if I add copy(List<RexNode>
newChildExps) to AbstractRelNode?

The use-case is as follows: when Calcite creates correlated plans, it
might reference the same source relation multiple times under
different correlation variable names.

For instance, here's the sample plan: Correlate(TableScan(A),
Filter(b.x=$cor1.x and b.y=$cor2.y, Scan(B))).
Latter in the processing, Calcite resolves those $cor1 and $cor2, and
I would like to rewrite the tree to make sure the same correlation
input is referenced by a single name (e.g. always as $cor1).

Currently I use RelShuttle, however I have to manually match
LogicalFilter/LogicalProject/LogicalJoin and create some (different!)
boiler-plate logic to call the appropriate copy(...) method.

-- 
Vladimir Sitnikov

Re: RelShuttle vs RexShuttle

[Vladimir Sitnikov <si...@gmail.com>]

I've created a "newbie" ticket for that:
https://issues.apache.org/jira/browse/CALCITE-479

Vladimir

Re: RelShuttle vs RexShuttle

[Julian Hyde <ju...@gmail.com>]

> What if we kill RelNode.getChildExps and add RelNode
> RelNode.accept(RexShuttle shuttle) instead?

+1 That method is very general-purpose. I like it.

It should respect the immutability of RelNodes and return a new
RelNode iff there are changes. Most RelNodes, except sub-classes of
project, filter, join, will just 'return this' right away.

The shuttle must make only very limited changes. Even changing the
output type of an expression should not be allowed. We can relax these
restrictions later, if justified.

On Mon, Nov 24, 2014 at 12:50 AM, Vladimir Sitnikov
<si...@gmail.com> wrote:
>> I don't know when it is safe to combine variables and when you need to define new ones
> Yes, that is a problem
> What if we kill RelNode.getChildExps and add RelNode
> RelNode.accept(RexShuttle shuttle) instead?
>
> This will result in:
> 1) Method RelNode.getChildExps will become less restrictive
> 2) RexShuttle transformations would become possible for RelNodes
> 3) New method would be consistent with RelNode RelNode.accept(RelShuttle)
> 4) AbstracRelNode.accept(RexShuttle) would be just "return this;".
>
> Vladimir

Re: RelShuttle vs RexShuttle

[Vladimir Sitnikov <si...@gmail.com>]

> I don't know when it is safe to combine variables and when you need to define new ones
Yes, that is a problem
What if we kill RelNode.getChildExps and add RelNode
RelNode.accept(RexShuttle shuttle) instead?

This will result in:
1) Method RelNode.getChildExps will become less restrictive
2) RexShuttle transformations would become possible for RelNodes
3) New method would be consistent with RelNode RelNode.accept(RelShuttle)
4) AbstracRelNode.accept(RexShuttle) would be just "return this;".

Vladimir

Re: RelShuttle vs RexShuttle

[Julian Hyde <ju...@gmail.com>]

The method you propose seems to be very specific to correlation variables, so at first I thought that the method should be in Correlate. I now agree that is belongs in RelNode/AbstractRelNode, because variables they are propagated through nodes of all types, even though they are mainly of interest to Correlate.

So, I would be OK having the method in AbstractRelNode as long as it is small (i.e. if there is complex code put it elsewhere such as RexUtil) and the method name and documentation clearly indicate that it is about correlation variables. "copy(List<RexNode>)" sounds general-purpose, which it isn't.

Also, take care when copying/combining RelNode trees with variables. I don't know when it is safe to combine variables and when you need to define new ones. I wonder whether your method needs an extra parameter to describe the mapping of old:new variables.

Julian



> On Nov 23, 2014, at 7:05 AM, Vladimir Sitnikov <si...@gmail.com> wrote:
> 
> Hi,
> 
> I need a kind of visitor that will be able to rewrite part of relation
> tree including rex expressions.
> RexShuttle does a good job when rewriting Rex expressions alone (e.g.
> calulations, casts, etc).
> 
> RelShuttle allows to visit Rel tree (e.g. Project/Filter/etc), however
> it does not allow to change expressions inside the tree.
> 
> Here's my question: does anybody object if I add copy(List<RexNode>
> newChildExps) to AbstractRelNode?
> 
> The use-case is as follows: when Calcite creates correlated plans, it
> might reference the same source relation multiple times under
> different correlation variable names.
> 
> For instance, here's the sample plan: Correlate(TableScan(A),
> Filter(b.x=$cor1.x and b.y=$cor2.y, Scan(B))).
> Latter in the processing, Calcite resolves those $cor1 and $cor2, and
> I would like to rewrite the tree to make sure the same correlation
> input is referenced by a single name (e.g. always as $cor1).
> 
> Currently I use RelShuttle, however I have to manually match
> LogicalFilter/LogicalProject/LogicalJoin and create some (different!)
> boiler-plate logic to call the appropriate copy(...) method.
> 
> -- 
> Vladimir Sitnikov