ulysses-you commented on code in PR #57986:
URL: https://github.com/apache/spark/pull/57986#discussion_r3795076530
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1761,9 +1761,39 @@ object CollapseWindow extends Rule[LogicalPlan] {
s1.zip(s2).forall(e => e._1.semanticEquals(e._2))
}
+ /**
+ * Returns true if the given window expression can still be evaluated
correctly when the rows
+ * of the partition are reordered, so that it can be merged into another
window with a different
+ * (non-empty) order spec.
+ *
+ * The frame determines whether reordering is safe. When the frame is the
whole partition
+ * (`UNBOUNDED PRECEDING` to `UNBOUNDED FOLLOWING`), it always covers all
the rows of the
+ * partition regardless of the ordering, so reordering changes only the
order in which the rows
+ * are seen, never which rows are in the frame. Since the order spec of the
window is empty,
+ * the query does not fix the row order, so evaluating its expressions under
any ordering
+ * yields a valid result, even though the value may differ for
order-dependent expressions
+ * such as `first`, `collect_list`, or floating-point `sum`/`avg`. On the
other hand, a bounded
+ * frame (e.g. `ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW`) is
order-sensitive: which
+ * rows are in the frame depends on the ordering, so even `count` or `sum`
would change value,
+ * and such a window must not be merged.
+ */
+ private def canEvaluateUnderAnyOrder(windowExpression: NamedExpression):
Boolean =
Review Comment:
Agreed a physical merge beside `PushDownLocalSort` would be architecturally
cleaner — it runs after `InferWindowGroupLimit` and needs no
order-insensitivity judgment. But that means moving the entire `CollapseWindow`
rule to the physical plan (rebuilding `windowFrameExpressionFactoryPairs`,
rewiring outputs), which is a much larger change. The WindowGroupLimit
interaction it avoids is, like Finding 1, a pre-existing property of the
logical rule (already present for same-order-spec merges), so I've kept the
logical rule with the config as a targeted escape hatch.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
