pitrou commented on code in PR #48601:
URL: https://github.com/apache/arrow/pull/48601#discussion_r2691204638
##########
docs/source/cpp/build_system.rst:
##########
@@ -235,10 +235,14 @@ will manage consistent versions of Arrow and its
dependencies.
Runtime Dependencies
====================
-While Arrow uses the OS-provided timezone database on Linux and macOS, it
-requires a user-provided database on Windows. You must download and extract the
-text version of the IANA timezone database and add the Windows timezone mapping
-XML. To download, you can use the following batch script:
+On Linux and macOS, Arrow uses the OS-provided timezone database. On Windows,
+Arrow uses the Windows timezone database when built with MSVC or recent MinGW
GCC
Review Comment:
I would perhaps phrase this as "On most platforms, Arrow uses the
OS-provided timezone database".
(because, well, who actually uses a CLang64-compiled Arrow on Windows?)
##########
cpp/src/arrow/config.h:
##########
@@ -86,12 +87,20 @@ const BuildInfo& GetBuildInfo();
ARROW_EXPORT
RuntimeInfo GetRuntimeInfo();
-struct GlobalOptions {
- /// Path to text timezone database. This is only configurable on Windows,
- /// which does not have a compatible OS timezone database.
+/// \deprecated Deprecated in 24.0.0. This struct is only needed for
+/// Windows builds with Clang/libc++ and will be removed once libc++
+/// supports std::chrono timezones.
+struct ARROW_DEPRECATED("Deprecated in 24.0.0. Only needed for Clang/libc++ on
Windows.")
+ GlobalOptions {
+ /// Path to text timezone database. This is only configurable on Windows
+ /// builds using Clang/libc++ which require the vendored date library.
std::optional<std::string> timezone_db_path;
};
+/// \deprecated Deprecated in 24.0.0. This function is only needed for
+/// Windows builds with Clang/libc++ and will be removed once libc++
+/// supports std::chrono timezones.
+ARROW_DEPRECATED("Deprecated in 24.0.0. Only needed for Clang/libc++ on
Windows.")
Review Comment:
I don't think we want to deprecate this function, as it could be useful for
other options in the future.
##########
python/pyarrow/tests/test_compute.py:
##########
@@ -2769,6 +2787,11 @@ def _check_temporal_rounding(ts, values, unit):
np.testing.assert_array_equal(result, expected)
+# TODO(GH-48743): Re-enable when Windows timezone issues are resolved
+# https://github.com/apache/arrow/issues/48743
Review Comment:
According to the issue, this is only with GCC, do we actually build PyArrow
with GCC on Windows?
##########
python/pyarrow/tests/test_misc.py:
##########
@@ -138,11 +138,13 @@ def import_arrow():
subprocess.check_call([sys.executable, "-c", code])
[email protected](sys.platform == "win32",
- reason="Path to timezone database is not configurable "
- "on non-Windows platforms")
-def test_set_timezone_db_path_non_windows():
- # set_timezone_db_path raises an error on non-Windows platforms
+# TODO(GH-48593): Remove when libc++ supports std::chrono timezone
+# https://github.com/apache/arrow/issues/48593
[email protected](
+ sys.platform == "win32",
+ reason="Timezone database path behavior varies by Windows build
configuration")
Review Comment:
Same question re: using libc++ with PyArrow on Windows.
##########
cpp/src/arrow/config.h:
##########
@@ -86,12 +87,20 @@ const BuildInfo& GetBuildInfo();
ARROW_EXPORT
RuntimeInfo GetRuntimeInfo();
-struct GlobalOptions {
- /// Path to text timezone database. This is only configurable on Windows,
- /// which does not have a compatible OS timezone database.
+/// \deprecated Deprecated in 24.0.0. This struct is only needed for
+/// Windows builds with Clang/libc++ and will be removed once libc++
+/// supports std::chrono timezones.
+struct ARROW_DEPRECATED("Deprecated in 24.0.0. Only needed for Clang/libc++ on
Windows.")
+ GlobalOptions {
+ /// Path to text timezone database. This is only configurable on Windows
+ /// builds using Clang/libc++ which require the vendored date library.
std::optional<std::string> timezone_db_path;
Review Comment:
Should perhaps only deprecate this member, not the entire struct?
##########
.github/workflows/cpp_extra.yml:
##########
@@ -377,6 +377,10 @@ jobs:
- name: Download Timezone Database
shell: bash
run: ci/scripts/download_tz_database.sh
+ - name: Install cmake
Review Comment:
Why do we need this now? Was it using a too old version of CMake?
##########
python/pyarrow/tests/test_compute.py:
##########
@@ -2397,7 +2406,14 @@ def test_strftime():
tsa = pa.array(ts, type=pa.timestamp("s", timezone))
result = pc.strftime(tsa, options=pc.StrftimeOptions(fmt + "%Z"))
expected = pa.array(ts.strftime(fmt + "%Z")).cast(result.type)
- assert result.equals(expected)
+ if sys.platform == "win32":
+ # TODO(GH-48767): On Windows, std::chrono returns GMT offset style
+ # https://github.com/apache/arrow/issues/48767
+ for val in result:
+ assert val.as_py() is None or "GMT" in val.as_py() \
+ or "UTC" in val.as_py()
Review Comment:
Ok, but that's not testing enough of the value. Can you check that it
corresponds to the expected timestamp formatting?
##########
docs/source/cpp/build_system.rst:
##########
@@ -235,10 +235,14 @@ will manage consistent versions of Arrow and its
dependencies.
Runtime Dependencies
====================
-While Arrow uses the OS-provided timezone database on Linux and macOS, it
-requires a user-provided database on Windows. You must download and extract the
-text version of the IANA timezone database and add the Windows timezone mapping
-XML. To download, you can use the following batch script:
+On Linux and macOS, Arrow uses the OS-provided timezone database. On Windows,
+Arrow uses the Windows timezone database when built with MSVC or recent MinGW
GCC
+(version 13+). However, when built with Clang/libc++ on Windows, Arrow requires
+a user-provided IANA timezone database.
+
+To download the timezone database for libc++ builds, you must download and
+extract the text version of the IANA timezone database and add the Windows
+timezone mapping XML. To download, you can use the following batch script:
.. literalinclude:: ../../../ci/appveyor-cpp-setup.bat
Review Comment:
Wait, does `appveyor-cpp-setup.bat` still exist? We migrated away from
AppVeyor.
##########
python/pyarrow/tests/test_util.py:
##########
@@ -212,6 +212,8 @@ def test_signal_refcycle():
assert wr() is None
+# TODO(GH-48593): Remove when libc++ supports std::chrono timezone
+# https://github.com/apache/arrow/issues/48593
Review Comment:
What does this skip have to do with libc++?
--
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]
