asolimando commented on code in PR #3757:
URL: https://github.com/apache/calcite/pull/3757#discussion_r1581902891
##########
core/src/main/java/org/apache/calcite/plan/RelCompositeTrait.java:
##########
@@ -116,4 +116,5 @@ public T trait(int i) {
public int size() {
return traits.length;
}
+
Review Comment:
Can you remove this modification? It seems to have slipped through by
mistake.
##########
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:
@jduo I have worked out two unit tests (convention and distribution in one
test, collations and convention in another as distribution cancels existing
collations), you can find them here:
https://github.com/apache/calcite/commit/9662b50f9391f790a6a909629d119c5a41e6fcec
Beware that the
`testPruneProjectInputOfAggregatePreservesConventionAndCollations` has lots of
extra print statements and that the last assertion over collections might be
imprecise (values and also satisfies is not enough as it doesn't cover cases
where there are extra collations like in our case).
Regarding 2., it seems off to me to, it's most probably a missing
functionality, it might even make sense to file a different issue and make the
current one depend on the new issue.
##########
core/src/test/java/org/apache/calcite/test/RelBuilderTest.java:
##########
@@ -1450,6 +1450,28 @@ private RexNode caseCall(RelBuilder b, RexNode ref,
RexNode... nodes) {
assertThat(root, hasTree(expected));
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE/issues/CALCITE-6340";>
+ * [CALCITE-6340] RelBuilder drops set conventions when aggregating over
duplicate
+ * projected fields.</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
Review Comment:
I guess we can drop both comments now, it's pretty clear from the new title
of the Jira ticket what's going on and what's the expected outcome
##########
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:
In addition, it seems the problem does not only pertain Convention but also
Distribution from the other comment from Adam, so we might want to change the
title again to "drops traits", sorry for not noticing before, I lost completely
the other conversation above.
As a general note, please refrain from force-pushing while the review is
on-going as it makes incremental reviews harder.
--
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]
