Repository navigation
GH-54641: [C++] Support locale directives in the vendored strptime on Windows - #54656
Conversation
…ime on Windows The vendored musl strptime, used on Windows, handles %a, %A, %b, %B, %h, %c, %p, %r, %x and %X only when nl_langinfo() is available, which it is not on Windows. Provide a C-locale replacement for the items used. ARROW_TEST_FALLBACK_LANGINFO compiles the replacement on other platforms. Drafted by Claude Code (an AI agent) and reviewed & approved by pearu.
|
|
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
I'm not sure whether we want to maintain a forked strptime() implementation or not but this change is small enough.
| #undef HAVE_LANGINFO | ||
|
|
||
| #ifndef _WIN32 | ||
| #if !defined(_WIN32) && !defined(ARROW_TEST_FALLBACK_LANGINFO) |
There was a problem hiding this comment.
Why do we need this change? It seems that ARROW_TEST_FALLBACK_LANGINFO isn't defined.
There was a problem hiding this comment.
It was only for compiling the fallback on Linux for manual testing. Removed; the fallback is tested by StrptimeLocaleDirectives on the Windows CI jobs.
| enum { ABDAY_1 = 0, DAY_1 = 7, ABMON_1 = 14, MON_1 = 26, AM_STR = 38, PM_STR = 39, | ||
| D_T_FMT = 40, D_FMT = 41, T_FMT = 42, T_FMT_AMPM = 43 }; | ||
| static const char *arrow_c_langinfo[] = { | ||
| "Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat", | ||
| "Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday", | ||
| "Jan", "Feb", "Mar", "Apr", "May", "Jun", | ||
| "Jul", "Aug", "Sep", "Oct", "Nov", "Dec", | ||
| "January", "February", "March", "April", "May", "June", | ||
| "July", "August", "September", "October", "November", "December", | ||
| "AM", "PM", | ||
| "%a %b %e %T %Y", "%m/%d/%y", "%H:%M:%S", "%I:%M:%S %p"}; |
There was a problem hiding this comment.
Could you mention where the values come from?
There was a problem hiding this comment.
They are the values of the POSIX locale (LC_TIME in POSIX.1, chapter 7), which musl's own C locale (c_time in src/locale/langinfo.c) uses in the same order; glibc's C locale has the same values. Added this to the comment.
github-actions
Bot
added
awaiting changes
…alues Drafted by Claude Code (an AI agent) and reviewed & approved by pearu.
|
|
github-actions
Bot
added
awaiting change review
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
+1
Could you try this before we merge this?
diff --git a/r/tests/testthat/test-dplyr-funcs-datetime.R b/r/tests/testthat/test-dplyr-funcs-datetime.R
index db036bcdea..2f99b6d750 100644
--- a/r/tests/testthat/test-dplyr-funcs-datetime.R
+++ b/r/tests/testthat/test-dplyr-funcs-datetime.R
@@ -238,8 +238,6 @@ test_that("strptime works for individual formats", {
collect()
)
- # Some formats are not supported on Windows
- skip_on_os("windows")
expect_equal(
strptime_test_df |>
arrow_table() |>
github-actions
Bot
added
awaiting merge
Drafted by Claude Code (an AI agent) and reviewed & approved by pearu.
|
Passed: https://github.com/apache/arrow/actions/runs/39413302527/job/118599671926?pr=54656#step:13:6966 I'll merge this. |
Rationale for this change
On Windows, the vendored musl
strptimedoes not handle%a,%A,%b,%B,%h,%c,%p,%r,%xand%X, because they neednl_langinfo(), which is not available there.What changes are included in this PR?
A C-locale replacement for
nl_langinfo(), restricted to the items the vendoredstrptimeuses, with the values of the POSIX locale.Are these changes tested?
Yes,
TimestampParser.StrptimeLocaleDirectivescovers each of these directives. On Windows it exercises the vendoredstrptime; elsewhere it runs against the systemstrptime.Are there any user-facing changes?
On Windows,
strptimeformats with these directives now parse.AI usage disclosure
Developed with AI assistance (Claude Code): the fix, the test and this description were AI-generated under my direction and reviewed line by line by me. I own and can debug these changes. No copyrighted material is included.