cloud-fan commented on code in PR #59135:
URL: https://github.com/apache/spark/pull/59135#discussion_r4281264122
##########
docs/sql-migration-guide.md:
##########
@@ -48,6 +48,7 @@ license: |
- Since Spark 4.4,
`spark.sql.sources.v2.bucketing.partitionKeyOrdering.enabled` defaults to
`true`. A V2 scan that reports a keyed partitioning but no explicit ordering
now also reports itself sorted by its partition key expressions, because every
row of such a partition evaluates them to the same value. Partition transforms
such as `days(ts)` or `bucket(8, id)` are left out of that ordering. This lets
Spark drop a `Sort` it would otherwise place above the scan. An aggregate that
groups by a leading run of the other key expressions, in key order, may also be
planned as a sort aggregate rather than a hash aggregate. This is because
`spark.sql.execution.replaceHashWithSortAgg` keys off the reported ordering. To
restore the previous behavior, set
`spark.sql.sources.v2.bucketing.partitionKeyOrdering.enabled` to `false`.
- Since Spark 4.4,
`spark.sql.sources.v2.bucketing.preserveKeyOrderingOnCoalesce.enabled` defaults
to `true`. When `GroupPartitionsExec` merges several input partitions that
share one partition key value into a single output partition, it now keeps sort
orders over the partition key expressions in the ordering it reports, since
those expressions are constant within the merged partition. Sort orders over
partition transforms such as `days(ts)` or `bucket(8, id)` are left out. Orders
over other columns are still dropped, as the concatenation invalidates them,
and a join that reduced the partition keys onto a common key space (see
`spark.sql.sources.v2.bucketing.allowCompatibleTransforms.enabled`) reports no
order, since the merged partitions then share only the reduced key. This can
remove a `Sort` downstream of the merge. To restore the previous behavior, set
`spark.sql.sources.v2.bucketing.preserveKeyOrderingOnCoalesce.enabled` to
`false`.
- Since Spark 4.4, the value columns of `UNPIVOT` and `Dataset.unpivot` merge
the nested nullability (array element, map value, and struct field nullability)
of all unpivoted values. Previously, a value column took the nested nullability
of the first listed value, so a `NULL` array element, map value, or struct
field of a later value could be returned as a non-`NULL` value such as `0`, and
`IS NULL` on it returned `false`. A view created by an earlier Spark version
over such an `UNPIVOT` may have recorded the non-nullable nested type in its
schema and can fail to read; recreate it with `CREATE OR REPLACE VIEW`. To
restore the previous behavior, set
`spark.sql.legacy.unpivot.useFirstValueNestedNullability` to `true`.
+- Since Spark 4.4, `date_add`, `date_sub`, `dateadd`, and adding or
subtracting an integer number of days to or from a date (for example `date + 1`
or `date - 1`) fail with `DATETIME_OVERFLOW` when the number of days since
1970-01-01 in the result does not fit in an `INT`, regardless of
`spark.sql.ansi.enabled`. Previously, the result silently wrapped around; for
example, `date_add(DATE'2020-01-01', 2147483647)` returned a date in year
-5877591. `try_add` and `try_subtract` on dates now return `NULL` in this case
instead of the wrapped date.
Review Comment:
**Nit (P3):** This entry lists `date_add`, `date_sub`, `dateadd`, and `date
+ 1` / `date - 1`. The check lives in `DateAdd`, though, and
`WindowEvaluatorFactoryBase.createBoundOrdering` also builds `DateAdd(expr,
boundOffset)` for a `RANGE` frame ordered by a DATE column with an integer
offset. Take this query:
```sql
SELECT d, count(*) OVER (ORDER BY d RANGE BETWEEN CURRENT ROW AND 2147483647
FOLLOWING)
FROM VALUES (DATE'2020-01-01') AS t(d)
```
It used to compute a wrapped, wrong frame bound. It now fails with
`DATETIME_OVERFLOW`, regardless of `spark.sql.ansi.enabled`. Someone upgrading
wouldn't connect that failure to this entry, since they never wrote `date + n`
themselves. Could you add a clause for DATE-ordered RANGE frames with integer
offsets? Day-interval forms (`date + INTERVAL n DAY` and DAY-interval frame
offsets) don't need a mention. A DAY interval is at most 106751991 days, so
they can't overflow for dates in the documented range.
**Verification:**
- **Inspection:** The rendered migration entry explicitly mentions RANGE
window frames over DATE columns with integer offsets. Its remaining claims
still match the implementation: it fails only on Int day-count overflow,
regardless of ANSI mode, and try_add/try_subtract return NULL.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala:
##########
@@ -510,7 +510,20 @@ class DateExpressionsSuite extends SparkFunSuite with
ExpressionEvalHelper {
DateAdd(Literal(Date.valueOf("2016-02-28")), negativeIntLit), -15910)
checkConsistencyBetweenInterpretedAndCodegen(DateAdd, DateType, ByteType)
checkConsistencyBetweenInterpretedAndCodegen(DateAdd, DateType, ShortType)
- checkConsistencyBetweenInterpretedAndCodegen(DateAdd, DateType,
IntegerType)
+ checkConsistencyBetweenInterpretedAndCodegenAllowingException(DateAdd,
DateType, IntegerType)
+ }
+
+ test("SPARK-59859: date_add fails on overflow instead of wrapping around") {
Review Comment:
**Nit (P3):** These tests cover `DateAdd`/`DateSub` directly. The PR also
says `try_add`/`try_subtract` on a date now return NULL on overflow, and that
depends on a separate composition. `TryAdd`'s non-numeric branch builds
`TryEval(Add(..., EvalMode.ANSI))`, the resolver lowers the inner `Add` to
`DateAdd`, and `TryEval` turns the exception into NULL. No test runs that path
with an overflowing date, and the existing date cases in `try_arithmetic.sql`
don't overflow. That branch also carries `// TODO: support TRY eval mode on
datetime arithmetic expressions.`. If someone later drops the `TryEval` wrapper
while implementing it, `try_add(DATE'2020-01-01', 2147483647)` would start
throwing and no test would fail. A couple of cases next to the `-- Date +
Integer` block in `try_arithmetic.sql` would pin this down, for example
`try_add(date'2021-01-01', 2147483647)` and `try_subtract(date'1900-01-01',
2147483647)`. Note that `inputs/nonansi/try_arithmetic.sql` imports that file.
That me
ans `results/nonansi/try_arithmetic.sql.out.java21` also has to be
regenerated, on JDK 21. A JDK 17 run leaves it stale, and that has needed
follow-up commits for this file before.
**Verification:**
- **Behavior:** SQL-level try_add and try_subtract on a date with a day
count that overflows the Int range return NULL, while the existing
non-overflowing date cases keep their current results.
- **Regression:** The new cases distinguish the fixed behavior from both
failure modes: a wrapped date (the pre-PR behavior) and a propagated
DATETIME_OVERFLOW (lost TryEval wrapping).
- **Regression:** SQLQueryTestSuite passes for try_arithmetic.sql and
nonansi/try_arithmetic.sql on JDK 21 as well as JDK 17, which shows the .java21
golden file was regenerated with the new cases.
--
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]
