sunchao commented on code in PR #7256:
URL: https://github.com/apache/datafusion-comet/pull/7256#discussion_r4446991346
##########
spark/src/main/scala/org/apache/spark/sql/comet/CometWindowExec.scala:
##########
@@ -75,16 +75,58 @@ object CometWindowExec extends
CometOperatorSerde[WindowExec] {
}
}
+ // Spark plans no sort below the window when its input already reports the
order the window
+ // needs. That order is relied on, by declaring it natively or by handing
the rows to Spark's
+ // window, only when mayReportFalseOrder trusts it: Spark's window would
group the rows of a
+ // false order wrongly, while the native one rejects an input whose order
it cannot see.
+ val requiredOrdering = op.requiredChildOrdering.head
+ val trustsInputOrder =
+ SortOrder.orderingSatisfies(op.child.outputOrdering, requiredOrdering) &&
+ !CometHashJoinExec.mayReportFalseInputOrder(op.child)
+
+ // The native window finds partitions and peers by comparing keys byte by
byte, so it would
+ // tell apart collated strings that Spark treats as equal, as
CometWindowGroupLimitExec notes.
+ val collated = (op.partitionSpec ++ op.orderSpec.map(_.child))
+ .filter(key => hasNonDefaultStringCollation(key.dataType))
+ if (collated.nonEmpty && trustsInputOrder) {
Review Comment:
The native window does not necessarily reject an untrusted order: with no
`PARTITION BY`, there are no partition-order requirements to fail. This now
also skips the collation fallback. I cached one sorted partition of 12 rows
with `v = 0..11` and `s` cycling through `A, a, B, b COLLATE UTF8_LCASE`,
broadcast `z = 0,1`, joined on `v % 2 <= z`, and selected `dense_rank() OVER
(ORDER BY s)` from the 18 joined rows. Spark gives ranks 1 and 2. This head
runs `CometWindowExec` and gives ranks 1 through 14, including ranks 1 and 3
for identical `A` values. A control with a physical sort above the same native
join matches Spark.
The join-order problem is already known; simply restoring the unconditional
Spark fallback also leaves the rows unsorted (that control gives four ranks).
Could this guard handle the unpartitioned case explicitly, preserving both
ordering and collation or rejecting the unsupported shape, and cover it in the
collated-window regression? The current partitioned/count fixture cannot expose
it.
--
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]
