You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Shuyi Chen <su...@gmail.com> on 2018/01/18 23:50:03 UTC
Question regarding ProjectRemoveRule
Hi,
I have a question regarding the ProjectRemoveRule. In Calcite 1980, a test
case was introduced below:
/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-1980">[CALCITE-1980]
* RelBuilder gives NPE if groupKey contains alias</a>.
*
* <p>Now, the alias does not cause a new expression to be added to the input,
* but causes the referenced fields to be renamed. */
@Test public void testAggregateProjectWithAliases() {
final RelBuilder builder = RelBuilder.create(config().build());
RelNode root =
builder.scan("EMP")
.project(builder.field("DEPTNO"))
.aggregate(
builder.groupKey(
builder.alias(builder.field("DEPTNO"), "departmentNo")))
.build();
final String expected = ""
+ "LogicalAggregate(group=[{0}])\n"
+ " LogicalProject(departmentNo=[$0])\n"
+ " LogicalProject(DEPTNO=[$7])\n"
+ " LogicalTableScan(table=[[scott, EMP]])\n";
assertThat(str(root), is(expected));
}
So according the ProjectRemoveRule, which will remove the top project, the
above REL will be optimized as the following:
LogicalAggregate(group=[{0}])
LogicalProject(DEPTNO=[$7])
LogicalTableScan(table=[[scott, EMP]])
But this will lose rename information "departmentNo". Is this expected, and
is it a problem that should get fixed? Please correct me if I am wrong.
Thanks a lot.
Shuyi
--
"So you have to trust that the dots will somehow connect in your future."
Re: Question regarding ProjectRemoveRule
Posted by Julian Hyde <jh...@apache.org>.
You say "fix this to make it work again” but I’m not sure it ever worked the way you want it to. Note that that block was added in https://issues.apache.org/jira/browse/CALCITE-92 <https://issues.apache.org/jira/browse/CALCITE-92>, which I think you would agree was a change for the better.
I’ve no objection to you looking for Projects inside a RelSubset. Remember that there might be several. You could also check for HepRelVertex.
Julian
> On Jan 18, 2018, at 5:57 PM, Shuyi Chen <su...@gmail.com> wrote:
>
> Thanks a lot, Julian. I was actually looking at the this code block in
> ProjectRemoveRule, it's actually trying to preserve the rename info of the
> top project (see the if block), so it may actually produce something like
> the following:
>
> LogicalAggregate(group=[{0}])
> LogicalProject(departmentNo=[$7])
> LogicalTableScan(table=[[scott, EMP]])
>
> public void onMatch(RelOptRuleCall call) {
> Project project = call.rel(0);
> assert isTrivial(project);
> RelNode stripped = project.getInput();
> if (stripped instanceof Project) {
> // Rename columns of child projection if desired field names are given.
> Project childProject = (Project) stripped;
> stripped = childProject.copy(childProject.getTraitSet(),
> childProject.getInput(), childProject.getProjects(),
> project.getRowType());
> }
> RelNode child = call.getPlanner().register(stripped, project);
> call.transformTo(child);
> }
>
>
> However, "stripped" is a RelSubset which contains a Project during the
> volcano planner optimization. So the if () code block won't be executed. Do
> you think we can fix this to make it work again? Thanks a lot.
>
> Shuyi
>
>
> On Thu, Jan 18, 2018 at 3:54 PM, Julian Hyde <jh...@apache.org> wrote:
>
>> Planner rules do their best to keep field names the same, but they don’t
>> guarantee it. If they did, they’d miss a lot of good opportunities to
>> optimize.
>>
>> By the way, the same goes for RelBuilder.
>>
>> Julian
>>
>>
>>> On Jan 18, 2018, at 3:50 PM, Shuyi Chen <su...@gmail.com> wrote:
>>>
>>> Hi,
>>>
>>> I have a question regarding the ProjectRemoveRule. In Calcite 1980, a
>> test
>>> case was introduced below:
>>>
>>> /** Test case for
>>> * <a href="https://issues.apache.org/jira/browse/CALCITE-1980">
>> [CALCITE-1980]
>>> * RelBuilder gives NPE if groupKey contains alias</a>.
>>> *
>>> * <p>Now, the alias does not cause a new expression to be added to the
>> input,
>>> * but causes the referenced fields to be renamed. */
>>> @Test public void testAggregateProjectWithAliases() {
>>> final RelBuilder builder = RelBuilder.create(config().build());
>>> RelNode root =
>>> builder.scan("EMP")
>>> .project(builder.field("DEPTNO"))
>>> .aggregate(
>>> builder.groupKey(
>>> builder.alias(builder.field("DEPTNO"),
>> "departmentNo")))
>>> .build();
>>> final String expected = ""
>>> + "LogicalAggregate(group=[{0}])\n"
>>> + " LogicalProject(departmentNo=[$0])\n"
>>> + " LogicalProject(DEPTNO=[$7])\n"
>>> + " LogicalTableScan(table=[[scott, EMP]])\n";
>>> assertThat(str(root), is(expected));
>>> }
>>>
>>> So according the ProjectRemoveRule, which will remove the top project,
>> the
>>> above REL will be optimized as the following:
>>>
>>> LogicalAggregate(group=[{0}])
>>> LogicalProject(DEPTNO=[$7])
>>> LogicalTableScan(table=[[scott, EMP]])
>>>
>>> But this will lose rename information "departmentNo". Is this expected,
>> and
>>> is it a problem that should get fixed? Please correct me if I am wrong.
>>> Thanks a lot.
>>>
>>> Shuyi
>>> --
>>> "So you have to trust that the dots will somehow connect in your future."
>>
>>
>
>
> --
> "So you have to trust that the dots will somehow connect in your future."
Re: Question regarding ProjectRemoveRule
Posted by Shuyi Chen <su...@gmail.com>.
Thanks a lot, Julian. I was actually looking at the this code block in
ProjectRemoveRule, it's actually trying to preserve the rename info of the
top project (see the if block), so it may actually produce something like
the following:
LogicalAggregate(group=[{0}])
LogicalProject(departmentNo=[$7])
LogicalTableScan(table=[[scott, EMP]])
public void onMatch(RelOptRuleCall call) {
Project project = call.rel(0);
assert isTrivial(project);
RelNode stripped = project.getInput();
if (stripped instanceof Project) {
// Rename columns of child projection if desired field names are given.
Project childProject = (Project) stripped;
stripped = childProject.copy(childProject.getTraitSet(),
childProject.getInput(), childProject.getProjects(),
project.getRowType());
}
RelNode child = call.getPlanner().register(stripped, project);
call.transformTo(child);
}
However, "stripped" is a RelSubset which contains a Project during the
volcano planner optimization. So the if () code block won't be executed. Do
you think we can fix this to make it work again? Thanks a lot.
Shuyi
On Thu, Jan 18, 2018 at 3:54 PM, Julian Hyde <jh...@apache.org> wrote:
> Planner rules do their best to keep field names the same, but they don’t
> guarantee it. If they did, they’d miss a lot of good opportunities to
> optimize.
>
> By the way, the same goes for RelBuilder.
>
> Julian
>
>
> > On Jan 18, 2018, at 3:50 PM, Shuyi Chen <su...@gmail.com> wrote:
> >
> > Hi,
> >
> > I have a question regarding the ProjectRemoveRule. In Calcite 1980, a
> test
> > case was introduced below:
> >
> > /** Test case for
> > * <a href="https://issues.apache.org/jira/browse/CALCITE-1980">
> [CALCITE-1980]
> > * RelBuilder gives NPE if groupKey contains alias</a>.
> > *
> > * <p>Now, the alias does not cause a new expression to be added to the
> input,
> > * but causes the referenced fields to be renamed. */
> > @Test public void testAggregateProjectWithAliases() {
> > final RelBuilder builder = RelBuilder.create(config().build());
> > RelNode root =
> > builder.scan("EMP")
> > .project(builder.field("DEPTNO"))
> > .aggregate(
> > builder.groupKey(
> > builder.alias(builder.field("DEPTNO"),
> "departmentNo")))
> > .build();
> > final String expected = ""
> > + "LogicalAggregate(group=[{0}])\n"
> > + " LogicalProject(departmentNo=[$0])\n"
> > + " LogicalProject(DEPTNO=[$7])\n"
> > + " LogicalTableScan(table=[[scott, EMP]])\n";
> > assertThat(str(root), is(expected));
> > }
> >
> > So according the ProjectRemoveRule, which will remove the top project,
> the
> > above REL will be optimized as the following:
> >
> > LogicalAggregate(group=[{0}])
> > LogicalProject(DEPTNO=[$7])
> > LogicalTableScan(table=[[scott, EMP]])
> >
> > But this will lose rename information "departmentNo". Is this expected,
> and
> > is it a problem that should get fixed? Please correct me if I am wrong.
> > Thanks a lot.
> >
> > Shuyi
> > --
> > "So you have to trust that the dots will somehow connect in your future."
>
>
--
"So you have to trust that the dots will somehow connect in your future."
Re: Question regarding ProjectRemoveRule
Posted by Julian Hyde <jh...@apache.org>.
Planner rules do their best to keep field names the same, but they don’t guarantee it. If they did, they’d miss a lot of good opportunities to optimize.
By the way, the same goes for RelBuilder.
Julian
> On Jan 18, 2018, at 3:50 PM, Shuyi Chen <su...@gmail.com> wrote:
>
> Hi,
>
> I have a question regarding the ProjectRemoveRule. In Calcite 1980, a test
> case was introduced below:
>
> /** Test case for
> * <a href="https://issues.apache.org/jira/browse/CALCITE-1980">[CALCITE-1980]
> * RelBuilder gives NPE if groupKey contains alias</a>.
> *
> * <p>Now, the alias does not cause a new expression to be added to the input,
> * but causes the referenced fields to be renamed. */
> @Test public void testAggregateProjectWithAliases() {
> final RelBuilder builder = RelBuilder.create(config().build());
> RelNode root =
> builder.scan("EMP")
> .project(builder.field("DEPTNO"))
> .aggregate(
> builder.groupKey(
> builder.alias(builder.field("DEPTNO"), "departmentNo")))
> .build();
> final String expected = ""
> + "LogicalAggregate(group=[{0}])\n"
> + " LogicalProject(departmentNo=[$0])\n"
> + " LogicalProject(DEPTNO=[$7])\n"
> + " LogicalTableScan(table=[[scott, EMP]])\n";
> assertThat(str(root), is(expected));
> }
>
> So according the ProjectRemoveRule, which will remove the top project, the
> above REL will be optimized as the following:
>
> LogicalAggregate(group=[{0}])
> LogicalProject(DEPTNO=[$7])
> LogicalTableScan(table=[[scott, EMP]])
>
> But this will lose rename information "departmentNo". Is this expected, and
> is it a problem that should get fixed? Please correct me if I am wrong.
> Thanks a lot.
>
> Shuyi
> --
> "So you have to trust that the dots will somehow connect in your future."