andygrove opened a new pull request, #23892: URL: https://github.com/apache/datafusion/pull/23892
## Which issue does this PR close? - Closes #. No single issue tracks these fixes. They were surfaced by an audit of `next_day` against the Spark source and a live Spark 4.2.0. The divergences this PR does *not* fix are filed separately and referenced from the test file: - https://github.com/apache/datafusion/issues/23889 — `next_day` rejects `STRING` and `TIMESTAMP` start dates that Spark accepts - https://github.com/apache/datafusion/issues/23890 — cross-version divergence in the ANSI default and error class between Spark 3.5 and 4.x - https://github.com/apache/datafusion/issues/23891 — `next_day` returns NULL for far-future start dates where Spark returns a value ## Rationale for this change Three divergences from Spark 4.2.0, in decreasing order of severity. **A panic reachable from SQL.** `next_date_for_day_of_week` used `NaiveDate + Duration`. `Date32Type::to_naive_date_opt` succeeds up to `chrono::NaiveDate::MAX` (epoch day `95026236`), and the subsequent addition then panics on overflow. Every epoch day in `95026230..=95026236` panicked, for any weekday: ```sql SELECT next_day(arrow_cast(95026236, 'Date32'), 'Mon'::string); -- thread panicked: `NaiveDate + TimeDelta` overflowed ``` This is reachable from plain SQL and from any `Date32` column containing such a value. **A whole argument shape was unsupported.** `SELECT next_day(<date literal>, <string column>)` fell into an `exec_err!` catch-all and raised a hard error on a query Spark evaluates normally. **ANSI mode was unimplemented.** Spark 4.0 made ANSI the default, and under it an unparseable day-of-week name raises `ILLEGAL_DAY_OF_WEEK` rather than returning NULL. The implementation carried `TODO` comments acknowledging this. ## What changes are included in this PR? - `next_date_for_day_of_week` uses `checked_add_signed`, trading the panic for NULL. NULL still diverges from Spark, which computes on the epoch day as `Int` with no calendar limit, so the residual gap is filed as #23891. - The match over `ColumnarValue` shapes is now exhaustive over all four combinations, so the `(Scalar, Array)` case works and the compiler enforces coverage. The `_ => exec_err!` catch-all is gone. - ANSI mode is honored via `datafusion.execution.enable_ansi_mode`, matching how `math/abs.rs` and `math/negative.rs` read it. Spark's per-row null intolerance is preserved: a NULL start date short-circuits to NULL before the day-of-week name is validated, so `next_day(NULL, 'xx')` is NULL in both modes. On the `(Array, Scalar)` path this is expressed as `date_array.null_count() < date_array.len()`, which is the closed form of exactly when the row-wise path raises. - The error message gains Spark 4.2.0's trailing period, matching the `ILLEGAL_DAY_OF_WEEK` template rather than Spark 3.5.8's older `IllegalArgumentException` text. - `spark_next_day` is split into `parse_day_of_week` and `next_date_for_day_of_week`. The old code round-tripped `"MO"` through `"MONDAY"` into `str::parse::<Weekday>()` with an unreachable `Err` arm; the replacement is a single table matching `DateTimeUtils.getDayOfWeekFromString` case for case. ## Are these changes tested? Yes. `next_day.slt` grows from 90 to 336 lines. Every expected value was observed by running the query against a local `pyspark==4.2.0` rather than derived from reading the Spark source. Coverage is organized as a product rather than a checklist, because the two bugs above hid in the gaps between axes: all four `ColumnarValue` shapes, each crossed with all-NULL, some-NULL and no-NULL inputs, each crossed with both ANSI modes. The `(Array, Scalar)` null-intolerance case in particular passes on the row-wise path and previously failed on the vectorized one, so testing only one shape would have missed it. Also covered: boundary dates through `+262142-12-31` including the entire previously-panicking range, day-name parsing (all three lengths, case insensitivity, whitespace padding, non-ASCII), and the argument type and arity errors. Divergences that are not fixed here are checked in as commented-out queries carrying the Spark 4.2.0 result and a link to the tracking issue. Uncommenting one is the contract for verifying a future fix. Verified with: ``` cargo fmt --all -- --check cargo clippy --all-targets --all-features -- -D warnings cargo test --test sqllogictests -- spark/datetime/next_day cargo test -p datafusion-spark --lib next_day ``` -- 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]
