xiedeyantu commented on code in PR #5040:
URL: https://github.com/apache/calcite/pull/5040#discussion_r3467213738
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -2504,6 +2504,11 @@ private RexNode convertOver(Blackboard bb, SqlNode node)
{
SqlCall call = (SqlCall) node;
bb.getValidator().deriveType(bb.scope, call);
SqlCall aggCall = call.operand(0);
+ @Nullable SqlNode filter = null;
Review Comment:
I rarely see the need for @Nullable in code.
##########
core/src/main/java/org/apache/calcite/rel/metadata/RelMdCollation.java:
##########
@@ -372,13 +373,16 @@ public static List<RelCollation> sort(RelCollation
collation) {
/** Helper method to determine a
* {@link org.apache.calcite.rel.core.Window}'s collation.
*
- * <p>A Window projects the fields of its input first, followed by the output
- * from each of its windows. Assuming (quite reasonably) that the
- * implementation does not re-order its input rows, then any collations of
its
- * input are preserved. */
+ * <p>A Window operator groups rows by PARTITION BY keys and sorts each
+ * partition by ORDER BY keys. The output order is therefore not defined by
+ * a simple collation in the general case, so we conservatively report no
+ * collations. */
public static @Nullable List<RelCollation> window(RelMetadataQuery mq,
RelNode input,
ImmutableList<Window.Group> groups) {
- return mq.collations(input);
+ Util.discard(mq);
+ Util.discard(input);
+ Util.discard(groups);
+ return Collections.emptyList();
Review Comment:
```suggestion
return Collections.emptyList();
```
##########
core/src/main/java/org/apache/calcite/rel/rules/CalcRelSplitter.java:
##########
@@ -113,7 +114,8 @@ public abstract class CalcRelSplitter {
this.program = calc.getProgram();
this.hints = calc.getHints();
this.cluster = calc.getCluster();
- this.traits = calc.getTraitSet();
+ this.traits = calc.getTraitSet()
+ .replaceIfs(RelCollationTraitDef.INSTANCE, Collections::emptyList);
Review Comment:
The collation reset should be scoped to the LogicalWindow creation rather
than applied in CalcRelSplitter’s constructor. The ordering issue is specific
to window nodes: a Window does not preserve or expose a simple output
collation, so it should not inherit the original Calc collation trait.
Suggested change:
ProjectToWindowRule.java:260
```
@Override protected RelNode makeRel(RelOptCluster cluster,
RelTraitSet traitSet,
RelBuilder relBuilder, RelNode input, RexProgram program,
List<RelHint> hints) {
checkArgument(program.getCondition() == null,
"WindowedAggregateRel cannot accept a condition");
+ traitSet = traitSet.replaceIfs(RelCollationTraitDef.INSTANCE,
+ Collections::emptyList);
return LogicalWindow.create(cluster, traitSet, relBuilder,
input,
program, hints);
}
```
This keeps the generic splitter behavior unchanged and makes the intent
local to the window-producing RelType.
--
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]
