Question about a possible bug in RelBuilder.aggregate_()

[Nick Riasanovsky <ni...@bodo.ai>]

Hello Calcite contributors,

I am working on a project that builds off of Calcite and I believe I may
have encountered a bug in RelBuilder.aggregate_(). The problem that I have
encountered is that this function is dropping traitSet information from my
the Project node when making a copy. In particular, line 2479 calls
project.copy() to prune any unused columns, but instead of reusing the
traitSet from the original projection it takes the traitSet directly from
the cluster.

In my example, this caused problems for me because I was propagating
convention information through the traitSet. I may be incorrectly relying
on the traitSet in this case and perhaps my copy implementation should be
improved, but I wanted to raise this as a possible concern in case anyone
else has encountered this issue. I am happy to submit a PR if you agree
this is a bug.

Thanks for you time,
Nick Riasanovsky

Re: Question about a possible bug in RelBuilder.aggregate_()

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

Use of “copy” methods has been worrying for a while. The methods are rarely used, and therefore inconsistently implemented. They may make assumptions that the new RelNode is fairly similar to the first (e.g. has the same number of expressions, and therefore has the same row type or field names) and those assumptions may not be valid.

There are many traits to be set up, and it is asking a lot for each copy method to set them correctly.

The requirement is often to create a RelNode of the same type (filter, aggregate, project etc.) and same convention (logical, enumerable, MongoDB, etc.) but different parameters. RelBuilder should solve this by asking the RelNode what its factory is (an interface in RelFactories), or indirectly get the factory via the calling convention, and then use that factory to create the new RelNode.

I think using a factory in combination with RelBuilder gives us the best shot of setting up traits correctly.

Julian



> On Oct 5, 2023, at 12:11 AM, LakeShen <sh...@gmail.com> wrote:
> 
> I have saw the code,I also find that the Project's RelTraitSet is
> from RelOptCluster's RelTraitSet,and the commits' history seems from
> CALCITE-4334 <https://issues.apache.org/jira/browse/CALCITE-4334>
> 
> I want to know what is the correct RelTraitSet in your scenario, are
> Project's RelTraitSet and RelOptCluster's RelTraitSet values different?
> What are they? I understand that RelOptCluster's RelTraitSet returns all
> RelTraitSets registered in Planner, and all of them would have the default
> values.If the previous Project has its own RelTraitSet, and it is different
> from RelOptCluster’s RelTraitSet, it may be a bug.
> 
> Best,
> LakeShen
> 
> Nick Riasanovsky <ni...@bodo.ai> 于2023年10月5日周四 11:29写道：
> 
>> Hello Calcite contributors,
>> 
>> I am working on a project that builds off of Calcite and I believe I may
>> have encountered a bug in RelBuilder.aggregate_(). The problem that I have
>> encountered is that this function is dropping traitSet information from my
>> the Project node when making a copy. In particular, line 2479 calls
>> project.copy() to prune any unused columns, but instead of reusing the
>> traitSet from the original projection it takes the traitSet directly from
>> the cluster.
>> 
>> In my example, this caused problems for me because I was propagating
>> convention information through the traitSet. I may be incorrectly relying
>> on the traitSet in this case and perhaps my copy implementation should be
>> improved, but I wanted to raise this as a possible concern in case anyone
>> else has encountered this issue. I am happy to submit a PR if you agree
>> this is a bug.
>> 
>> Thanks for you time,
>> Nick Riasanovsky
>> 

Re: Question about a possible bug in RelBuilder.aggregate_()

[LakeShen <sh...@gmail.com>]

I have saw the code,I also find that the Project's RelTraitSet is
from RelOptCluster's RelTraitSet,and the commits' history seems from
CALCITE-4334 <https://issues.apache.org/jira/browse/CALCITE-4334>

I want to know what is the correct RelTraitSet in your scenario, are
Project's RelTraitSet and RelOptCluster's RelTraitSet values different?
What are they? I understand that RelOptCluster's RelTraitSet returns all
RelTraitSets registered in Planner, and all of them would have the default
values.If the previous Project has its own RelTraitSet, and it is different
from RelOptCluster’s RelTraitSet, it may be a bug.

Best,
LakeShen

Nick Riasanovsky <ni...@bodo.ai> 于2023年10月5日周四 11:29写道：

> Hello Calcite contributors,
>
> I am working on a project that builds off of Calcite and I believe I may
> have encountered a bug in RelBuilder.aggregate_(). The problem that I have
> encountered is that this function is dropping traitSet information from my
> the Project node when making a copy. In particular, line 2479 calls
> project.copy() to prune any unused columns, but instead of reusing the
> traitSet from the original projection it takes the traitSet directly from
> the cluster.
>
> In my example, this caused problems for me because I was propagating
> convention information through the traitSet. I may be incorrectly relying
> on the traitSet in this case and perhaps my copy implementation should be
> improved, but I wanted to raise this as a possible concern in case anyone
> else has encountered this issue. I am happy to submit a PR if you agree
> this is a bug.
>
> Thanks for you time,
> Nick Riasanovsky
>