Copilot commented on code in PR #51315:
URL: https://github.com/apache/arrow/pull/51315#discussion_r3997141248
##########
cpp/src/arrow/config.cc:
##########
@@ -91,7 +103,11 @@ RuntimeInfo GetRuntimeInfo() {
Status Initialize(const GlobalOptions& options) noexcept {
ARROW_SUPPRESS_DEPRECATION_WARNING
if (options.timezone_db_path.has_value()) {
-#if !USE_OS_TZDB
+#if defined(ARROW_USE_STD_CHRONO) && ARROW_USE_STD_CHRONO
+ return Status::Invalid(
+ "Arrow was built with C++20 std::chrono and uses the OS timezone
database, "
+ "so a downloaded database cannot be provided at runtime.");
Review Comment:
`ARROW_USE_STD_CHRONO` does not mean every datetime consumer uses the
standard backend: the build deliberately keeps `vendored/datetime.cpp` when
Gandiva is enabled, and Gandiva still calls
`arrow_vendored::date::locate_zone`/`zoned_time`. Rejecting `timezone_db_path`
here prevents configuring the vendored database that Gandiva needs, so
std-chrono + Gandiva can fail at runtime. Keep the vendored configuration path
for that combination (using a generated Gandiva/backend flag) instead of
treating the whole library as OS-tzdb-only.
##########
cpp/src/arrow/config.cc:
##########
@@ -77,11 +83,17 @@ RuntimeInfo GetRuntimeInfo() {
MakeSimdLevelString([&](int64_t flags) { return
cpu_info->IsSupported(flags); });
info.detected_simd_level =
MakeSimdLevelString([&](int64_t flags) { return
cpu_info->IsDetected(flags); });
+#if defined(ARROW_USE_STD_CHRONO) && ARROW_USE_STD_CHRONO
+ // GH-51267: std::chrono builds always use the OS timezone database.
+ info.using_os_timezone_db = true;
+ info.timezone_db_path = std::optional<std::string>();
+#else
Review Comment:
In a CMake subproject with `ARROW_DEFINE_OPTIONS=OFF`, the generated config
intentionally leaves `ARROW_USE_STD_CHRONO` undefined, so `chrono_internal.h`
still selects `std::chrono` on Windows via its platform fallback. `config.cc`
does not include that header, so this preprocessor branch is skipped and such a
binary reports `using_os_timezone_db=false` (and continues exposing the
vendored timezone configuration) even though its chrono backend uses the OS
database. Please use a shared/resolved backend predicate here so `RuntimeInfo`
and `Initialize` match the header-level fallback as well.
##########
cpp/cmake_modules/CheckStdChrono.cmake:
##########
@@ -0,0 +1,93 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Resolve the ARROW_USE_STD_CHRONO option (AUTO, ON or OFF) into a boolean.
+#
+# GH-51267 tracks removing the vendored datetime fallback once all supported
+# toolchains provide working C++20 chrono timezone support. Until then:
+# - AUTO keeps the historical platform default: std::chrono is used on Windows
+# toolchains whose standard library provides working C++20 chrono timezone
+# support, and the vendored datetime fallback is used everywhere else.
+# - ON opts into std::chrono unconditionally, failing the configure step when
+# the toolchain does not provide working C++20 chrono timezone support.
+# - OFF always uses the vendored datetime fallback.
+#
+# The resolved value is consumed via arrow/util/config.h (ARROW_USE_STD_CHRONO)
+# by arrow/util/chrono_internal.h and to decide whether the vendored datetime
+# implementation is built.
+
+include(CheckCXXSourceCompiles)
+
+# When Arrow is consumed as a CMake subproject, ARROW_USE_STD_CHRONO is not
+# defined; skip detection and let arrow/util/chrono_internal.h fall back to its
+# default backend selection (vendored datetime fallback).
+if(DEFINED ARROW_USE_STD_CHRONO)
+if(NOT "${ARROW_USE_STD_CHRONO}" MATCHES "^(AUTO|ON|OFF)$")
+ message(FATAL_ERROR "ARROW_USE_STD_CHRONO must be one of AUTO, ON or OFF "
+ "(got \"${ARROW_USE_STD_CHRONO}\")")
+endif()
+
+set(_ARROW_STD_CHRONO_TEST_SOURCE
+ "
+#include <chrono>
+#if !defined(__cpp_lib_chrono) || __cpp_lib_chrono < 201907L
+# error \"C++20 chrono timezone support (__cpp_lib_chrono >= 201907L) is
unavailable\"
+#endif
+int main() { return 0; }
Review Comment:
This probe only checks the feature-test macro and links an empty `main`, but
the selected branch unconditionally includes `<format>`, calls
`std::chrono::locate_zone`, and uses `std::vformat`. A standard library can
advertise `__cpp_lib_chrono >= 201907L` while one of those headers/APIs is
unavailable, so `ARROW_USE_STD_CHRONO=ON` can pass configure and then fail to
build. Make the CMake and Meson probes compile/link the APIs actually used by
`chrono_internal.h`.
##########
r/R/arrow-package.R:
##########
@@ -180,6 +180,13 @@ s3_finalizer <- new.env(parent = emptyenv())
configure_tzdb <- function() {
if (requireNamespace("tzdb", quietly = TRUE)) {
+ if (runtime_info()[[3]] == "true") {
+ # GH-51267: builds reading the OS timezone database (C++20 std::chrono,
+ # or the vendored library built against the OS tzdata) cannot use a
+ # downloaded database, and their timezones already work — skip the
+ # vendored path instead of surfacing a false startup failure.
+ return(invisible())
+ }
Review Comment:
Because this check is nested under `requireNamespace()`, a std::chrono build
with no `tzdb` package still falls into the existing `else` and emits
“Timezones will not be available”, even though `using_os_timezone_db` is true.
Move the backend check before package detection (or guard the `else`) so the
OS-backend path is silent regardless of whether `tzdb` is installed.
--
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]
