Skip to content
Open
6 changes: 2 additions & 4 deletions docs/source/contributor-guide/roadmap.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,13 @@ Native window execution runs by default (`spark.comet.exec.window.enabled`). The
`dense_rank`, `row_number`, `percent_rank`, `cume_dist`, `ntile`), value functions (`lag`, `lead`, `nth_value`,
`first_value`, `last_value`), and the `count`, `min`, `max`, `sum`, and `avg` aggregates are accelerated.
Remaining work is to close the gaps that still fall back to Spark: statistical aggregates (`stddev`, `variance`,
`corr`, `covar`) and `collect_list` / `collect_set` as window functions ([#4766]), `RANGE` frames with explicit
decimal offsets ([#4834]), `first_value` / `last_value` on `RANGE` frames with a literal offset ([#4835]), and
non-literal `lag` / `lead` default values ([#4268]). See the
`corr`, `covar`) and `collect_list` / `collect_set` as window functions ([#4766]), `first_value` / `last_value` on
`RANGE` frames with a literal offset ([#4835]), and non-literal `lag` / `lead` default values ([#4268]). See the
[window function compatibility guide](../user-guide/latest/compatibility/operators.md) for the complete list of
supported functions, frames, and fallback cases.

[#4268]: https://github.com/apache/datafusion-comet/issues/4268
[#4766]: https://github.com/apache/datafusion-comet/issues/4766
[#4834]: https://github.com/apache/datafusion-comet/issues/4834
[#4835]: https://github.com/apache/datafusion-comet/issues/4835

## Native Lambda Evaluation
Expand Down
7 changes: 5 additions & 2 deletions docs/source/user-guide/latest/compatibility/operators.md
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,11 @@ incorrect result. When any single window expression in a `WindowExec` falls back
([#7007](https://github.com/apache/datafusion-comet/issues/7007)). From precision 28 the native average also
differs from Spark once the running sum overflows precision 38
([#5418](https://github.com/apache/datafusion-comet/issues/5418)).
- `RANGE` frame with an explicit offset when the `ORDER BY` column is `DECIMAL`
([#4834](https://github.com/apache/datafusion-comet/issues/4834)).
- `RANGE` frame with an offset when the `ORDER BY` column is `DECIMAL` and the bound can overflow `Decimal128`, that
is, when the largest value of the `ORDER BY` type plus the offset exceeds 2^127 - 1 unscaled. Only `DECIMAL(38, s)`
can reach that, and only with an offset above about 7.01 * 10^37 unscaled. Spark computes the bound exactly, while
the native frame treats an overflowing bound as the edge of the partition and so takes in `NULL` keys sorted
there ([#5022](https://github.com/apache/datafusion-comet/issues/5022)).
- `RANGE` frame with an offset above 2144550751 days when the `ORDER BY` column is `DATE`. Spark adds the offset
to the date as an `Int` that wraps on overflow, while the native frame treats an overflowing bound as the edge of
the partition. Smaller offsets cannot wrap for any date between 0001-01-01 and 9999-12-31
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)) =>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] [P2] Preserve Spark's descending decimal-offset negation

Could this fallback also account for offsets whose magnitude changes when Spark negates them for descending order? With d DECIMAL(35,0) containing [N,0], where N = 10000000000000000000000000000000001, COUNT(*) OVER (ORDER BY d DESC RANGE BETWEEN CURRENT ROW AND N FOLLOWING) returns [1,1] on Spark 4.1.3 under both ANSI settings. Spark's UnaryMinus(offset) rounds to -10000000000000000000000000000000000, leaving the first row's boundary at 1. This guard returns true, and rangeBoundLiteral preserves the positive offset exactly, so the native frame subtracts N, reaches 0, and returns [2,1]. The previous blanket fallback preserved Spark's result. Please retain fallback for affected descending offsets or serialize Spark's effective offset, with a regression containing rows at the differing boundary.

Evidence: Reproduction: create t with SELECT CAST(str AS DECIMAL(35,0)) AS d FROM VALUES ('10000000000000000000000000000000001'), ('0') AS v(str), then run SELECT d, COUNT(*) OVER (ORDER BY d DESC RANGE BETWEEN CURRENT ROW AND 10000000000000000000000000000000001 FOLLOWING) AS c FROM t ORDER BY d DESC. Spark 4.1.3 produced counts [1,1] with ANSI true and false. The compiled exact-head isDecimalRangeOffsetSafe returned true and the serialized d.abs retained all 35 digits. The lockfile-matching DataFusion 55.2.0/Arrow 59.3.0 WindowFrameContext, using CurrentRow and the exact decimal Following scalar, produced [2,1]. Also reproduced with DECIMAL(35,34) and offset 1.0000000000000000000000000000000001. Spark 3.5.9 returned [2,1], confirming the version-dependent behavior. Harnesses and logs are under /tmp/comet4987-83a6-review/.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

withFallbackReason(
windowExpr,
"RANGE frame with explicit offset on DECIMAL ORDER BY is not supported")
s"RANGE frame on ${dt.catalogString} ORDER BY with an offset whose bound can " +
"overflow Decimal128 is not supported")
return None
case Some(DateType) if !Seq(lb, ub).forall(isDateRangeOffsetSafe) =>
withFallbackReason(
Expand Down Expand Up @@ -700,6 +700,35 @@ object CometWindowExec extends CometOperatorSerde[WindowExec] {
}
}

private val MaxDecimal128Unscaled: BigInt = BigInt(2).pow(127) - 1

// DataFusion rescales both operands to the larger scale and adds them as
// i128, so the bound cannot overflow when the largest value the ORDER BY
// type holds plus the offset fits in an i128.
private def isDecimalRangeOffsetSafe(bound: Expression, orderType: DecimalType): Boolean =
bound match {
case _: SpecialFrameBoundary => true
case e =>
val offset =
try {
e.eval()
} catch {
// Rejected when the bound is serialized.
case _: Exception => return true
}
offset match {
case d: Decimal if d.scale >= 0 && orderType.scale >= 0 =>
val scale = math.max(orderType.scale, d.scale)
val maxValue = (BigInt(10).pow(orderType.precision) - 1) *
BigInt(10).pow(scale - orderType.scale)
val offsetValue =
BigInt(d.toJavaBigDecimal.unscaledValue).abs * BigInt(10).pow(scale - d.scale)
maxValue + offsetValue <= MaxDecimal128Unscaled
case _: Decimal => false
case _ => true
}
}

// Folds a RANGE frame bound expression to a constant and serializes its
// magnitude as a typed Literal proto. Spark encodes PRECEDING/FOLLOWING via
// the sign of the literal (negative => PRECEDING, positive => FOLLOWING),
Expand Down
14 changes: 6 additions & 8 deletions spark/src/test/resources/sql-tests/windows/window_functions.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +251 to +255

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
-- 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.
-- 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.

-- ============================================================

query expect_fallback(RANGE frame with explicit offset on DECIMAL ORDER BY is not supported)
query
SELECT COUNT(*) OVER (PARTITION BY 1
ORDER BY CAST(1 AS DECIMAL(10, 0)) DESC
RANGE BETWEEN CURRENT ROW AND 1 FOLLOWING) AS c
Expand Down
Loading
Loading