andygrove opened a new pull request, #23885:
URL: https://github.com/apache/datafusion/pull/23885
## Which issue does this PR close?
N/A
## Rationale for this change
`spark_next_day` did a surprising amount of work per row to turn a day name
into
a `Weekday`:
```rust
let day_of_week = day_of_week.to_uppercase(); // heap allocation +
Unicode casing
let day_of_week = match day_of_week.as_str() {
"MO" | "MON" | "MONDAY" => Some("MONDAY"), // 21 literals
...
};
let day_of_week = day_of_week.parse::<Weekday>(); // second
case-insensitive match
```
So every row allocated a `String`, matched it against 21 literals to produce
*another* string literal, and then re-parsed that literal back into the enum
the
match had already identified.
On top of that, `next_day(col, 'MONDAY')` — the common form, where the day
name
is a constant — ran all of the above once per row even though the answer is
the
same for the whole batch.
## What changes are included in this PR?
- The literal match now yields a `Weekday` directly, so the round-trip
through
`"MONDAY"` and `parse::<Weekday>()` is gone.
- `parse_day_of_week` upper-cases ASCII input in a `[u8; 9]` stack buffer
(`"WEDNESDAY"` is the longest accepted name, so anything longer cannot
match
and returns early). No allocation on this path.
- Non-ASCII input still goes through `str::to_uppercase`. That is deliberate:
`to_uppercase` is Unicode-aware, matching Spark's
`toUpperCase(Locale.ROOT)`,
and handling it with ASCII casing would change which strings match. For
example U+017F (`ſ`, long s) upper-cases to `S`, so `"ſun"` is accepted by
Spark and by this function, on both the old and new code.
- `spark_next_day` now takes a `Weekday` rather than a `&str`, which lets the
array-plus-scalar branch parse the name once before the loop instead of
inside
`unary_opt`.
No change to which inputs are accepted or what they return.
## Are these changes tested?
`spark/datetime/next_day.slt` asserts concrete dates across abbreviations,
full
names, mixed case, invalid names, and NULL inputs; all 53 `spark/datetime`
sqllogictest files pass, along with the 261 `datafusion-spark` unit tests.
The existing `next_day_rejects_whitespace_padded_day_names` test now targets
`parse_day_of_week` directly, since that is where the rejection lives. Three
tests were added:
- `parse_day_of_week_is_case_insensitive` — abbreviations and full names in
lower, upper, and mixed case.
- `parse_day_of_week_rejects_unknown_names` — empty, too short, too long
(past the stack-buffer bound), non-day words, and non-ASCII.
- `parse_day_of_week_matches_unicode_uppercasing` — pins that the ASCII fast
path and the Unicode fallback accept the same set, using the `ſ` → `S` case
above. This is the test that would catch the fast path silently narrowing
what `next_day` accepts.
The benchmark used below is added separately in #23882, so the baseline can
be
measured on `main` before this change lands.
### Benchmarks
Criterion, `apache/main` @ `f1ab86dad` as baseline. Median of the reported
change interval.
| Benchmark | 1024 | 8192 |
| --- | --- | --- |
| `next_day/scalar_day` | −47.2% | −46.0% |
| `next_day/array_day` | −37.6% | −33.7% |
`scalar_day` is `next_day(col, 'MONDAY')`; `array_day` passes the day name
as a
column, so it still parses per row and gains only from the allocation
removal.
## Are there any user-facing changes?
No. `next_day` accepts the same inputs and returns the same results.
--
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]
