Add DatePart enum 1-indexed variants#9965
Conversation
`DayOfWeekMonday1` (`1..=7`, Mon=1) matches PostgreSQL's `isodow`; `DayOfWeekSunday1` (`1..=7`, Sun=1) matches Spark's `dayofweek`. `FromStr` accepts `isodow` and `dayofweek1` for these. `FromStr` previously rejected `isodow` because mapping it to `DayOfWeekMonday0` (`0..=6`) would silently shift PostgreSQL's `1..=7` semantics. The new variants map faithfully, so downstream wrappers (e.g. datafusion-spark's `date_part`) can drop their post-hoc `+1` over the result array and reduce to pure alias remapping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The three `*_invalid_parts` arrays were hand-maintained spot-check lists that had drifted from the impls: every `ExtractDatePartExt` for Time, Interval, and Duration arrays rejects `YearISO` and `WeekISO`, but the tests never asserted it. Add both variants to all three arrays so the negative-test coverage matches the actual rejection contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 0-indexed pair was already `Sun0, Mon0` (Sunday first), but the 1-indexed pair I added landed as `Mon1, Sun1`. Flip the new pair to match — every match block, alias table, and test array now reads `Sun0, Mon0, Sun1, Mon1`, so the four variants line up Sunday-first across both halves. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| "isoweek" => Self::WeekISO, | ||
| "d" | "day" | "days" => Self::Day, | ||
| "dow" | "dayofweek" => Self::DayOfWeekSunday0, | ||
| "dayofweek1" => Self::DayOfWeekSunday1, |
There was a problem hiding this comment.
dayofweek1 seems non-standard here?
There was a problem hiding this comment.
Yeah, I'll take a look at similar enums in other projects. The reason to add it here is for upstream DataFusion Spark temporal function support.
sparkdow?
There was a problem hiding this comment.
MySQL/BigQuery/SQL Server/Oracle have dow Sunday = 1 as well
There was a problem hiding this comment.
Yeah, I'll take a look at similar enums in other projects. The reason to add it here is for upstream DataFusion Spark temporal function support.
sparkdow?
How would this look like downstream? As in, would DataFusion need to provide sparkdow or dayofweek1 as a string to be parsed here? Is there no way to provide the enum variant directly, for example?
2 participants
Which issue does this PR close?
DatePart1-indexed variants #9964.Rationale for this change
Remove extra computations done on the datafusion side for temporal functions.
What changes are included in this PR?
DatePartvariantAre these changes tested?
Are there any user-facing changes?
No