ARROW-13168: [C++][R] Enable runtime timezone database for Windows #12536

wjones127 · 2022-03-01T23:44:56Z

This allows for runtime configuration of the timezone database on Windows for C++ and R. Python will be handled later because it's available timezone libraries use the binary rather than text format, which is not yet supported the vendored date library.

For R, Windows will only support the "C" locale, since (as far as I can tell) that's the only locale supported by the MingW std::locale implementation. I think R itself gets around this by implementing a completely custom version of strftime() and friends.

github-actions · 2022-03-01T23:45:16Z

https://issues.apache.org/jira/browse/ARROW-13168

wjones127 · 2022-03-02T22:24:52Z

So timezone database seems to work, but methods that rely on std::local currently error with:

Error (test-dplyr-funcs-datetime.R:344:3): extract month from timestamp
Error: Invalid: Cannot find locale 'English_United States.1252': locale::facet::_S_create_c_locale name not valid
C:/Users/voltron/arrow/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc:1085  GetLocale(options.locale)
C:/Users/voltron/arrow/cpp/src/arrow/compute/kernels/scalar_temporal_unary.cc:1105  Make(ctx, *in.type)
C:/Users/voltron/arrow/cpp/src/arrow/compute/exec.cc:700  kernel_->exec(kernel_ctx_, batch, &out)
C:/Users/voltron/arrow/cpp/src/arrow/compute/exec.cc:641  ExecuteBatch(batch, listener)
C:/Users/voltron/arrow/cpp/src/arrow/compute/exec/expression.cc:547  executor->Execute(arguments, &listener)
C:/Users/voltron/arrow/cpp/src/arrow/compute/exec/expression.cc:533  ExecuteScalarExpression(call->arguments[i], input, exec_context)
C:/Users/voltron/arrow/cpp/src/arrow/compute/exec/project_node.cc:91  ExecuteScalarExpression(simplified_expr, target, plan()->exec_context())
C:/Users/voltron/arrow/cpp/src/arrow/compute/exec/exec_plan.cc:484  iterator_.Next()
C:/Users/voltron/arrow/cpp/src/arrow/record_batch.cc:336  ReadNext(&batch)
C:/Users/voltron/arrow/cpp/src/arrow/record_batch.cc:347  ReadAll(&batches)

These do work if you set Sys.setlocale("LC_TIME", "C"). If I don't find a fix for this, I may consider only supporting the "C" locale on Windows.

r/R/arrow-package.R

r/R/dplyr-funcs-datetime.R

r/tests/testthat/test-dplyr-funcs-datetime.R

wjones127 · 2022-03-22T21:45:13Z

CI failure is unrelated Flight error.

wjones127 · 2022-03-22T21:46:21Z

cc @pitrou

rok · 2022-03-22T23:04:53Z

ci/appveyor-cpp-setup.bat

+@rem Download IANA Timezone Database for unit tests
+@rem
+@rem (Doc section: Download timezone database)
+curl https://data.iana.org/time-zones/releases/tzdata2021e.tar.gz --output tzdata.tar.gz


Will this always be the same DB that R and Python will be using?

No that 2021e is a version, which won't necessarily align with the R and Python ones in the future. I don't think it matters that we update it, unless the format changes somehow. This is just for testing that it works, and we don't ship it.

But the R unit tests use the one provided by the tzdb package, so we are testing that as well.

Currently US and EU have DST, but they plan to abolish it soon. At that point we will have Python and R DBs with fresh DSTless times and arrow c++ using DST for tests. In isolation that's ok, but we do have tests comparing pandas and pyarrow results and similar for lubridate. It's not a big problem for sure, but if there is like a tzdata-latest.tar.gz that would be great.

The times in the past won't change, so unless we test against a random "now", updates to the timezone database (such as for DST policy changes) shouldn't impact those tests?

Yeah this should be fine as is. We will never be testing between timezone databases.

We currently test arrow vs pandas tzdb in CI. We have a test for a timestamp in 2033 and if it is in DST and DST is abolished it will error if we'll be using a pre-abolishment db with arrow and post-abolishment db with pandas. This is super irrelevant and as it's a simple fix and I'm sorry for wasting your time :).

Ah got it. We'll address in the follow up PR where we'll have PyArrow use tzdb. For now the python timestamp tests are skipped on Windows

arrow/python/pyarrow/tests/test_compute.py

Lines 1915 to 1917 in d327f69

if sys.platform == 'win32':

# TODO: We should test on windows once ARROW-13168 is resolved.

pytest.skip('Timezone database is not available on Windows yet')

pitrou

Thanks a lot for this @wjones127 . Here are some comments.

cpp/src/arrow/config.h

cpp/src/arrow/config.cc

cpp/src/arrow/public_api_test.cc

docs/source/cpp/build_system.rst

docs/source/developers/cpp/windows.rst

pitrou · 2022-03-23T13:27:30Z

docs/source/cpp/build_system.rst

+.. literalinclude:: ../../../ci/appveyor-cpp-setup.bat
+   :language: cmd
+   :start-after: @rem (Doc section: Download timezone database)
+   :end-before: @rem (Doc section: Download timezone database)


I'd rather if you copied the relevant snippet here, we shouldn't ideally rely on the contents of CI scripts (which may contain specific quirks that irrelevant to normal user setups) for the public docs.

Since we are pulling a specific part of the script, we should easily be able to avoid quirks? I like the idea of having our build instructions tested in CI. And we reference the CI scripts regularly when providing build instructions.

At the very least, if we get to a point where the public instructions and CI instructions diverge, we can easily separate them.

pitrou · 2022-03-23T13:29:29Z

cpp/src/arrow/compute/kernels/scalar_temporal_test.cc

@@ -1875,6 +1870,9 @@ TEST_F(ScalarTemporalTest, StrftimeCLocale) {
 }

 TEST_F(ScalarTemporalTest, StrftimeOtherLocale) {
+#ifdef _WIN32
+  GTEST_SKIP() << "There is a known bug in strftime for locales on Windows (ARROW-15922)";


Is this on Windows or specifically MinGW? i.e., would the non-MinGW Windows CI pass if you remove this skip? I'm wondering if we can narrow the condition.

If failed on Windows 2019 C++17, which uses MSVC. But seems like it was actually passing on MinGW. So maybe I can try skipping for just MSVC?

That's a bit weird, MinGW is just a different compiler but targetting the same runtime libraries...

Actually, I think the test is likely skipping on the LocaleExists("fr_FR.UTF-8") condition; IIRC MinGW doesn't support locales apart from "C" and "POSIX". Sadly, no indication in CI whether the test was skipped: https://github.com/apache/arrow/runs/5504310182?check_suite_focus=true

pitrou · 2022-03-23T13:33:27Z

docs/source/cpp/build_system.rst

+   :start-after: @rem (Doc section: Download timezone database)
+   :end-before: @rem (Doc section: Download timezone database)
+
+By default, the timezone database will be detected at ``%USERPROFILE%\Downloads\tzdata``,


Is this a reliable location? Is there a risk that the downloads folder gets cleared from time to time?

This is the behavior of the vendored datetime library, not something we chose. I think it's a fine default for testing and in production applications I expect users will manually specify a more appropriate path at runtime.

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

jonkeane

A few things I noticed in CI

r/src/config.cpp

cpp/src/arrow/config.cc

Co-authored-by: Jonathan Keane <jkeane@gmail.com>

DavisVaughan · 2022-03-25T20:04:31Z

ci/scripts/download_tz_database.sh

+set -ex
+
+# Download database
+curl https://data.iana.org/time-zones/releases/tzdata2021e.tar.gz --output ~/Downloads/tzdata2021e.tar.gz


FWIW, they just released 2022a a few days ago

r/R/arrow-package.R

Co-authored-by: Davis Vaughan <davis@rstudio.com>

r/tests/testthat/test-dplyr-funcs-datetime.R

jonkeane · 2022-03-28T19:54:02Z

Are there any outstanding comments that we need to resolve before merging?

The failures in CI both look unrelated. I'm happy to merge if no one objects

ursabot · 2022-03-28T21:33:07Z

Benchmark runs are scheduled for baseline = 919d113 and contender = f4dfd6c. f4dfd6c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.34% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️1.02% ⬆️0.81%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

wjones127 · 2022-03-28T22:29:08Z

Follow-up Jira created: https://issues.apache.org/jira/browse/ARROW-16054

github-actions bot added Component: C++ Component: R labels Mar 1, 2022

wjones127 commented Mar 3, 2022

View reviewed changes

r/R/arrow-package.R Outdated Show resolved Hide resolved

nealrichardson reviewed Mar 3, 2022

View reviewed changes

r/R/dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

nealrichardson reviewed Mar 3, 2022

View reviewed changes

r/tests/testthat/test-dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

wjones127 force-pushed the ARROW-13168-timezone-database branch from d8aeb45 to 2171629 Compare March 8, 2022 18:08

wjones127 added 17 commits March 9, 2022 14:33

First pass at R bindings

7c8cbc2

Enable R timezone unit tests

d92b1ab

Get timezone tests passing in R; locale still a problem

bf82303

Force Windows time locale to be C

be6b344

Try to fix onLoad warning

a1be197

Try requireNamespace

89ce1bf

Add tzdb download to CI

191885c

First pass at R bindings

b58d102

Enable R timezone unit tests

05d1dee

Get timezone tests passing in R; locale still a problem

48c5584

Force Windows time locale to be C

647e3ac

PR feedback on R bindings

22fc930

More fixes for CI scripts

1c609aa

Add section to developer docs for downloading

64af70c

Add new test to illuminate issue

410c2e5

Make tzdb test skip if not found in downloads

5a33d30

Get other test running

ab5d038

wjones127 force-pushed the ARROW-13168-timezone-database branch from 497aab6 to ab5d038 Compare March 10, 2022 22:27

Fix some failures

668c8cc

wjones127 mentioned this pull request Mar 11, 2022

Invalide unicode character in month name from to_stream HowardHinnant/date#726

Open

wjones127 added 2 commits March 11, 2022 14:07

Just skip the locale test for now

32df619

Cleanup

7ebf6a8

wjones127 marked this pull request as ready for review March 14, 2022 17:09

dragosmg mentioned this pull request Mar 21, 2022

ARROW-15802 [R] bindings for lubridate::make_datetime() and lubridate::make_date() #12622

Closed

2 tasks

Add underscore to WIN32

d883a48

wjones127 requested a review from nealrichardson March 22, 2022 21:45

rok reviewed Mar 22, 2022

View reviewed changes

pitrou reviewed Mar 23, 2022

View reviewed changes

wjones127 and others added 8 commits March 23, 2022 10:57

Update docs/source/cpp/build_system.rst

60ccf65

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Update cpp/src/arrow/public_api_test.cc

ece8ce0

Co-authored-by: Antoine Pitrou <pitrou@free.fr>

Address PR feedback

7da26f6

Link docs

2471139

Use environment variable for public api tests

843cdb6

Allow tests set location via environment variable

29dc875

Document environment variable

c78d3cd

Cleanup

fe6469d

jonkeane reviewed Mar 24, 2022

View reviewed changes

r/src/config.cpp Outdated Show resolved Hide resolved

cpp/src/arrow/config.cc Outdated Show resolved Hide resolved

wjones127 and others added 2 commits March 24, 2022 19:40

Apply suggestions from code review

9f97016

Co-authored-by: Jonathan Keane <jkeane@gmail.com>

Fix type that led to empty string

ad161ae

DavisVaughan reviewed Mar 25, 2022

View reviewed changes

r/R/arrow-package.R Outdated Show resolved Hide resolved

Update r/R/arrow-package.R

62e8178

Co-authored-by: Davis Vaughan <davis@rstudio.com>

jonkeane reviewed Mar 25, 2022

View reviewed changes

r/tests/testthat/test-dplyr-funcs-datetime.R Show resolved Hide resolved

jonkeane closed this in f4dfd6c Mar 28, 2022

rok mentioned this pull request Mar 30, 2022

ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone #12240

Closed

asfimport mentioned this pull request Nov 23, 2022

[C++] Timezone database configuration and access #28868

Closed

rok mentioned this pull request May 24, 2023

GH-35040: [Python] Skip test_cast_timestamp_to_string on Windows because it requires tz database #35735

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-13168: [C++][R] Enable runtime timezone database for Windows #12536

ARROW-13168: [C++][R] Enable runtime timezone database for Windows #12536

wjones127 commented Mar 1, 2022 •

edited

Loading

github-actions bot commented Mar 1, 2022

wjones127 commented Mar 2, 2022

wjones127 commented Mar 22, 2022

wjones127 commented Mar 22, 2022

rok Mar 22, 2022

wjones127 Mar 23, 2022

rok Mar 23, 2022

jorisvandenbossche Mar 25, 2022

wjones127 Mar 25, 2022

rok Mar 28, 2022

wjones127 Mar 28, 2022

pitrou left a comment

pitrou Mar 23, 2022

wjones127 Mar 23, 2022

pitrou Mar 23, 2022

wjones127 Mar 23, 2022

pitrou Mar 23, 2022

wjones127 Mar 23, 2022 •

edited

Loading

pitrou Mar 23, 2022

wjones127 Mar 23, 2022

jonkeane left a comment

DavisVaughan Mar 25, 2022

jonkeane commented Mar 28, 2022

ursabot commented Mar 28, 2022 •

edited

Loading

wjones127 commented Mar 28, 2022

	if sys.platform == 'win32':
	# TODO: We should test on windows once ARROW-13168 is resolved.
	pytest.skip('Timezone database is not available on Windows yet')

ARROW-13168: [C++][R] Enable runtime timezone database for Windows #12536

ARROW-13168: [C++][R] Enable runtime timezone database for Windows #12536

Conversation

wjones127 commented Mar 1, 2022 • edited Loading

github-actions bot commented Mar 1, 2022

wjones127 commented Mar 2, 2022

wjones127 commented Mar 22, 2022

wjones127 commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjones127 Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonkeane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonkeane commented Mar 28, 2022

ursabot commented Mar 28, 2022 • edited Loading

wjones127 commented Mar 28, 2022

wjones127 commented Mar 1, 2022 •

edited

Loading

wjones127 Mar 23, 2022 •

edited

Loading

ursabot commented Mar 28, 2022 •

edited

Loading