asolimando commented on code in PR #3757:
URL: https://github.com/apache/calcite/pull/3757#discussion_r1571121711
##########
core/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -2502,9 +2502,10 @@ private RelBuilder
pruneAggregateInputFieldsAndDeduplicateAggCalls(
newProjects.add(project.getProjects().get(i));
builder.add(project.getRowType().getFieldList().get(i));
}
+
r =
- project.copy(cluster.traitSet(), project.getInput(), newProjects,
- builder.build());
+ project.copy(project.getTraitSet().apply(targetMapping),
project.getInput(),
Review Comment:
You don't need any mapping here, the root cause of the issue is that the
previous code is taking the trait set from the cluster instead of the one of
the `Project` it is trying to copy, this is all you need:
```
r = project.copy(project.getTraitSet(), project.getInput(), newProjects,
builder.build());
```
##########
core/src/test/java/org/apache/calcite/test/RelBuilderTest.java:
##########
@@ -547,6 +547,31 @@ private void
checkSimplify(UnaryOperator<RelBuilder.Config> transform,
assertThat(root, matcher);
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/projects/CALCITE/issues/CALCITE-6340";>
+ * [CALCITE-6340] RelBuilder always creates Project with Convention.NONE
during aggregate_.</a>.
+ */
+ @Test void testPruneProjectInputOfAggregatePreservesTraitSet() {
Review Comment:
The test should be moved close to other tests around aggregate, after
`testAggregateEliminatesDuplicateCalls3`, for instance
##########
core/src/test/java/org/apache/calcite/test/RelBuilderTest.java:
##########
@@ -547,6 +547,31 @@ private void
checkSimplify(UnaryOperator<RelBuilder.Config> transform,
assertThat(root, matcher);
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/projects/CALCITE/issues/CALCITE-6340";>
+ * [CALCITE-6340] RelBuilder always creates Project with Convention.NONE
during aggregate_.</a>.
Review Comment:
The Jira link is wrong (you have `projects` instead of `browse` when you
copy it from the project view in Jira):
```suggestion
* <a
href="https://issues.apache.org/jira/browse/CALCITE/issues/CALCITE-6340";>[CALCITE-6340]
* RelBuilder always creates Project with Convention.NONE during
aggregate_.</a>.
```
The ticket title should strive to be as much high-level as possible, the
current title goes too deep into Calcite internals IMO.
Factoring in also the new minimal reproducer below, I would go for something
along the line of "RelBuilder drops set conventions when aggregating over
duplicate projected fields", WDYT?
In case you change the Jira title, please remember to be consistent here,
and in the final commit message too when squashing before merging (only when
the review process is over, though).
##########
core/src/test/java/org/apache/calcite/test/RelBuilderTest.java:
##########
@@ -547,6 +547,31 @@ private void
checkSimplify(UnaryOperator<RelBuilder.Config> transform,
assertThat(root, matcher);
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/projects/CALCITE/issues/CALCITE-6340";>
+ * [CALCITE-6340] RelBuilder always creates Project with Convention.NONE
during aggregate_.</a>.
+ */
+ @Test void testPruneProjectInputOfAggregatePreservesTraitSet() {
+ final RelBuilder builder = createBuilder(config ->
config.withPruneInputOfAggregate(true));
+
+ // This issue only occurs when projecting more columns than there are
fields and putting
+ // an aggregate over that projection.
+ RelNode root =
+ builder.scan("DEPT")
+ .adoptConvention(EnumerableConvention.INSTANCE)
+ .project(builder.alias(builder.field(0), "a"),
+ builder.alias(builder.field(1), "b"),
+ builder.alias(builder.field(2), "c"),
+ builder.alias(builder.field(1), "d"))
+ .aggregate(builder.groupKey(0, 1, 2, 1),
+ builder.aggregateCall(SqlStdOperatorTable.SUM,
+ builder.field(0)))
+ .build();
+
+ // Verify that the project under the aggregate kept the
EnumerableConvention.INSTANCE trait.
+
assertTrue(root.getInput(0).getTraitSet().contains(EnumerableConvention.INSTANCE));
Review Comment:
Test is not really minimal, you just need a single duplicate projected
field, as follows:
```suggestion
final RelNode root = builder.scan("DEPT")
.adoptConvention(EnumerableConvention.INSTANCE)
.project(builder.alias(builder.field(0), "a"),
builder.alias(builder.field(0), "b"))
.aggregate(builder.groupKey(0), builder.aggregateCall(
SqlStdOperatorTable.SUM, builder.field(0))).build();
assertTrue(root.getInput(0).getTraitSet().contains(EnumerableConvention.INSTANCE));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
