0lai0 commented on code in PR #4987:
URL: https://github.com/apache/datafusion-comet/pull/4987#discussion_r4423898222
##########
spark/src/main/scala/org/apache/spark/sql/comet/CometWindowExec.scala:
##########
@@ -393,30 +393,30 @@ object CometWindowExec extends
CometOperatorSerde[WindowExec] {
}
}
- // Comet's native window planner ships RANGE frame offsets as
- // ScalarValue::Int64, which the DECIMAL comparator can't handle: Spark
- // decimal arithmetic widens precision on +/-, so the computed boundary
- // (e.g. Decimal(11,0)) doesn't match the current value's precision
- // (e.g. Decimal(10,0)) and the comparator fails with "Uncomparable
- // values". Fall back for DECIMAL until the native side normalizes
- // precision. DATE is now handled natively by wrapping the offset as
- // IntervalDayTime in the planner. UNBOUNDED / CURRENT ROW bounds don't
- // evaluate the offending arithmetic and stay native regardless.
+ // DataFusion computes a RANGE frame bound as `current +/- offset` and
treats
+ // a bound that overflows as the edge of the partition, which also takes in
+ // NULL keys sorted at that edge. Spark's bound never overflows that way,
so
+ // fall back for the offsets that can overflow. UNBOUNDED / CURRENT ROW
+ // bounds skip the arithmetic and stay native.
//
// For DATE, Spark computes the bound with DateAdd, an Int addition that
- // wraps silently on overflow, while DataFusion treats an overflowing bound
- // as the partition edge. Over dates in Spark's supported range the two
only
- // disagree when the offset is large enough to wrap, so fall back for those
- // offsets. https://github.com/apache/datafusion-comet/issues/5022
+ // wraps silently on overflow. Over dates in Spark's supported range the
two
+ // only disagree when the offset is large enough to wrap, so fall back for
+ // those offsets.
+ //
+ // For DECIMAL, Spark's DecimalAddNoOverflowCheck keeps the exact bound,
+ // while DataFusion adds in Decimal128 and overflows past i128.
+ // https://github.com/apache/datafusion-comet/issues/5022
f match {
case SpecifiedWindowFrame(RangeFrame, lb, ub)
if !lb.isInstanceOf[SpecialFrameBoundary] ||
!ub.isInstanceOf[SpecialFrameBoundary] =>
windowExpr.windowSpec.orderSpec.headOption.map(_.dataType) match {
- case Some(_: DecimalType) =>
+ case Some(dt: DecimalType) if !Seq(lb,
ub).forall(isDecimalRangeOffsetSafe(_, dt)) =>
Review Comment:
Thanks @sunchao, good catch. Fixed in the latest commit.
For a descending `ORDER BY`, `CometWindowExec` now evaluates a `DECIMAL`
frame offset as `UnaryMinus(UnaryMinus(bound))`: the same negation
`WindowEvaluatorFactoryBase` applies, then negated back. On Scala 2.13 the
first negation rounds to 34 significant digits exactly as Spark does, and
negating the rounded value back is exact, so the native frame gets Spark's
effective offset with the sign of the bound. On Scala 2.12 neither step rounds,
which matches Spark 3.5 returning `[2,1]`. Both the serialized offset and the
overflow guard use this value, since rounding can also push an offset up past
the `Decimal128` limit.
Added `RANGE offset on DECIMAL ORDER BY uses the offset Spark evaluates`,
with your `DECIMAL(35,0)` case and the `DECIMAL(35,34)` one, over rows at the
differing boundary (`N`, the smallest positive value, `0`) and a `NULL`, across
ASC/DESC, `PRECEDING`/`FOLLOWING` and both ANSI settings. It passes on the
Spark 4.1 and 3.5 profiles.
This also changed one existing expectation: under a descending `ORDER BY`,
the exact `DECIMAL(38,10)` limit rounds up past the limit when Spark negates
it, so it now falls back, and the at-the-limit test uses the largest 34-digit
offset under the limit for descending frames.
--
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]
