perf: parse next_day's day-of-week without allocating, and once per batch#23885
Draft
andygrove wants to merge 1 commit into
Draft
perf: parse next_day's day-of-week without allocating, and once per batch#23885andygrove wants to merge 1 commit into
andygrove wants to merge 1 commit into
Conversation
…atch spark_next_day called day_of_week.to_uppercase() on every row -- a heap allocation plus full Unicode case mapping -- matched the result against 21 string literals, mapped that to another literal, then parsed that literal back into a Weekday with a second case-insensitive match. The match now yields a Weekday directly, and ASCII input is upper-cased in a stack buffer. Non-ASCII input keeps the allocating to_uppercase path, since that is Unicode-aware and dropping it would change which strings match. For next_day(col, 'MONDAY') the name is constant across the batch, so it is now parsed once before the loop rather than on every row. Scalar day-of-week -47%, column day-of-week -34%, against the benchmark added in apache#23882.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23885 +/- ##
========================================
Coverage 80.65% 80.65%
========================================
Files 1091 1092 +1
Lines 371031 371135 +104
Branches 371031 371135 +104
========================================
+ Hits 299256 299344 +88
- Misses 53935 53943 +8
- Partials 17840 17848 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
andygrove
marked this pull request as draft
July 25, 2026 17:19
Member
Author
|
Moving to draft. Fixing some bugs first. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
N/A
Rationale for this change
spark_next_daydid a surprising amount of work per row to turn a day name intoa
Weekday:So every row allocated a
String, matched it against 21 literals to produceanother 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 nameis 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?
Weekdaydirectly, so the round-trip through"MONDAY"andparse::<Weekday>()is gone.parse_day_of_weekupper-cases ASCII input in a[u8; 9]stack buffer(
"WEDNESDAY"is the longest accepted name, so anything longer cannot matchand returns early). No allocation on this path.
str::to_uppercase. That is deliberate:to_uppercaseis Unicode-aware, matching Spark'stoUpperCase(Locale.ROOT),and handling it with ASCII casing would change which strings match. For
example U+017F (
ſ, long s) upper-cases toS, so"ſun"is accepted bySpark and by this function, on both the old and new code.
spark_next_daynow takes aWeekdayrather than a&str, which lets thearray-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.sltasserts concrete dates across abbreviations, fullnames, mixed case, invalid names, and NULL inputs; all 53
spark/datetimesqllogictest files pass, along with the 261
datafusion-sparkunit tests.The existing
next_day_rejects_whitespace_padded_day_namestest now targetsparse_day_of_weekdirectly, since that is where the rejection lives. Threetests were added:
parse_day_of_week_is_case_insensitive— abbreviations and full names inlower, 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 fastpath and the Unicode fallback accept the same set, using the
ſ→Scaseabove. This is the test that would catch the fast path silently narrowing
what
next_dayaccepts.The benchmark used below is added separately in #23882, so the baseline can be
measured on
mainbefore this change lands.Benchmarks
Criterion,
apache/main@f1ab86dadas baseline. Median of the reportedchange interval.
next_day/scalar_daynext_day/array_dayscalar_dayisnext_day(col, 'MONDAY');array_daypasses the day name as acolumn, so it still parses per row and gains only from the allocation removal.
Are there any user-facing changes?
No.
next_dayaccepts the same inputs and returns the same results.