rich7420 commented on code in PR #4987:
URL: https://github.com/apache/datafusion-comet/pull/4987#discussion_r4348645683
##########
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:
Could an ASC lower bound like `-10000000000000000000000000000000001BD
FOLLOWING` lose precision here? `d.abs` in `rangeBoundLiteral` rounds beyond 34
digits in Scala 2.13. Maybe use `Decimal(d.toJavaBigDecimal.abs, d.precision,
d.scale)` instead?
##########
spark/src/test/resources/sql-tests/windows/window_functions.sql:
##########
@@ -248,16 +248,14 @@ ORDER BY cate, val_date
-- Ported from Spark's typeCoercion/native/windowFrameCoercion.sql:
-- SELECT COUNT(*) OVER (PARTITION BY 1 ORDER BY cast(1 as decimal(10, 0))
DESC
-- RANGE BETWEEN CURRENT ROW AND 1 FOLLOWING) FROM t
--- Comet falls back because Spark decimal arithmetic widens precision on
--- +/-, so the native frame-boundary arithmetic produces e.g. Decimal(11,0)
--- while the current row stays Decimal(10,0), and DataFusion's comparator
--- fails with "Uncomparable values". Once the native planner preserves the
--- ORDER BY column's precision when computing RANGE boundaries, the guard
--- in CometWindowExec can be removed and this test will start failing
--- because Comet stops falling back — that's the signal to re-enable it.
+-- Runs natively: Spark's WindowFrameTypeCoercion casts the frame offset to
+-- the same Decimal type as the ORDER BY column, so the boundary arithmetic
+-- keeps the current row's precision and DataFusion's comparator has matching
+-- operands. The DATE case in 1.11 keeps an IntegerType offset instead, which
+-- the native planner coerces to IntervalDayTime.
Review Comment:
nit: Could we explain why this now passes? The bound still widens to
DECIMAL(11,0); I think it works because DataFusion compares decimals with
matching scales regardless of precision.
```suggestion
-- Runs natively: Spark casts the frame offset to the ORDER BY decimal type.
-- Boundary arithmetic still widens precision (e.g. DECIMAL(11,0)), but
-- DataFusion compares Decimal128 values with matching scales regardless of
-- precision. The DATE case in 1.11 keeps an IntegerType offset instead,
which
-- the native planner coerces to IntervalDayTime.
```
--
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]
