pitrou commented on code in PR #51216: URL: https://github.com/apache/arrow/pull/51216#discussion_r3970044893
########## cpp/src/arrow/util/chrono_config_internal.h: ########## @@ -0,0 +1,56 @@ +// 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. + +#pragma once + +#include <chrono> + +// Share backend selection with the vendored implementation without including its +// headers. datetime.h undefines macros needed when compiling the implementation. +// +// On Windows, MSVC's standard library uses the system timezone database, while +// libstdc++ reads tzdata files (using TZDIR). Libraries without the C++20 timezone +// APIs, including older libc++, still require the vendored date library. +// +// Use the standard backend by default. Builds may explicitly define +// ARROW_USE_STD_CHRONO to 0 or 1 when they need to select a backend. +// +// Automatically disable the default for libraries without the C++20 timezone APIs. +// On non-Windows, older libstdc++ versions also need the fallback because of +// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116110 (fully fixed in GCC 16.2). +// Check library macros, not __GNUC__, so Clang using libstdc++ agrees with GCC. +// The datestamp distinguishes 16.2 (2026-08-07) from 16.1 and early snapshots. +// Keep the existing Windows backend selection unchanged. Review Comment: "Unchanged" from what? ########## cpp/src/arrow/util/chrono_config_internal.h: ########## @@ -0,0 +1,56 @@ +// 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. + +#pragma once + +#include <chrono> + +// Share backend selection with the vendored implementation without including its +// headers. datetime.h undefines macros needed when compiling the implementation. +// +// On Windows, MSVC's standard library uses the system timezone database, while +// libstdc++ reads tzdata files (using TZDIR). Libraries without the C++20 timezone +// APIs, including older libc++, still require the vendored date library. +// +// Use the standard backend by default. Builds may explicitly define +// ARROW_USE_STD_CHRONO to 0 or 1 when they need to select a backend. Review Comment: Should this be moved up? ########## cpp/src/arrow/util/chrono_config_internal.h: ########## @@ -0,0 +1,56 @@ +// 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. + +#pragma once + +#include <chrono> + +// Share backend selection with the vendored implementation without including its +// headers. datetime.h undefines macros needed when compiling the implementation. Review Comment: What is "the implementation"? Which one? Did you generate this using AI? ########## cpp/src/arrow/util/chrono_config_internal.h: ########## @@ -0,0 +1,56 @@ +// 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. + +#pragma once + +#include <chrono> + +// Share backend selection with the vendored implementation without including its +// headers. datetime.h undefines macros needed when compiling the implementation. +// +// On Windows, MSVC's standard library uses the system timezone database, while +// libstdc++ reads tzdata files (using TZDIR). Libraries without the C++20 timezone +// APIs, including older libc++, still require the vendored date library. +// +// Use the standard backend by default. Builds may explicitly define +// ARROW_USE_STD_CHRONO to 0 or 1 when they need to select a backend. +// +// Automatically disable the default for libraries without the C++20 timezone APIs. Review Comment: "Disable the default" is a bit cryptic. Please be explciit. ########## cpp/src/arrow/util/chrono_internal.h: ########## @@ -151,22 +127,172 @@ inline const time_zone* locate_zone(std::string_view tz_name) { inline const time_zone* current_zone() { return std::chrono::current_zone(); } -// Formatting support - streams directly using C++20 std::vformat_to -// Provides: direct streaming, stream state preservation, chaining, rich format specifiers +namespace detail { + +// Argument positions passed to std::vformat by to_stream below. +enum class FormatArgument : char { + ZonedTime = '0', + TimeOfDay = '1', + TimeOfDayCount = '2', +}; + +template <typename CharT> Review Comment: Why all the `CharT` templating? Are we supporting anything else than 8-bit `char`? ########## cpp/src/arrow/util/chrono_internal.h: ########## @@ -151,22 +127,172 @@ inline const time_zone* locate_zone(std::string_view tz_name) { inline const time_zone* current_zone() { return std::chrono::current_zone(); } -// Formatting support - streams directly using C++20 std::vformat_to -// Provides: direct streaming, stream state preservation, chaining, rich format specifiers +namespace detail { + +// Argument positions passed to std::vformat by to_stream below. +enum class FormatArgument : char { + ZonedTime = '0', + TimeOfDay = '1', + TimeOfDayCount = '2', +}; + +template <typename CharT> +void AppendEscapedLiteral(std::basic_string<CharT>* out, CharT value) { + out->push_back(value); + if (value == CharT{'{'} || value == CharT{'}'}) { + out->push_back(value); + } +} + +// These are the directives accepted by Arrow's existing strftime syntax. Treat +// all others as literals to preserve compatibility. +template <typename CharT> +bool IsSupportedStrftimeSpecifier(CharT modifier, CharT specifier) { + const auto contains = [specifier](const char* candidates) { + for (; *candidates != '\0'; ++candidates) { + if (specifier == static_cast<CharT>(*candidates)) return true; + } + return false; + }; + if (modifier == CharT{}) { + return contains("aAbBhcCxdeDFgGHIjmMprRSTuUVWwXyYzZ"); + } + if (modifier == CharT{'E'}) { + return contains("cCxXyYz"); + } + if (modifier == CharT{'O'}) { + return contains("deHImMSuUVwWyz"); + } + return false; +} + +template <typename CharT> +void AppendChronoField(std::basic_string<CharT>* out, FormatArgument argument, + CharT specifier, CharT modifier = CharT{}) { + *out += {CharT{'{'}, static_cast<CharT>(argument), CharT{':'}, CharT{'L'}, CharT{'%'}}; + if (modifier != CharT{}) out->push_back(modifier); + *out += {specifier, CharT{'}'}}; +} + +template <typename CharT> +void AppendLocalizedField(std::basic_string<CharT>* out, FormatArgument argument) { + *out += {CharT{'{'}, static_cast<CharT>(argument), CharT{':'}, CharT{'L'}, CharT{'}'}}; +} + +template <typename CharT> +std::basic_string<CharT> ToChronoFormat(const CharT* fmt, bool use_microseconds_suffix) { + std::basic_string<CharT> out; + while (*fmt != CharT{}) { + if (*fmt != CharT{'%'}) { + AppendEscapedLiteral(&out, *fmt++); + continue; + } + + ++fmt; + if (*fmt == CharT{}) { + AppendEscapedLiteral(&out, CharT{'%'}); + break; + } + + CharT modifier{}; + if (*fmt == CharT{'E'} || *fmt == CharT{'O'}) { + modifier = *fmt++; + if (*fmt == CharT{}) { + AppendEscapedLiteral(&out, CharT{'%'}); + AppendEscapedLiteral(&out, modifier); + break; + } + } + const CharT specifier = *fmt++; + + if (modifier == CharT{}) { + switch (specifier) { + case CharT{'%'}: + AppendEscapedLiteral(&out, CharT{'%'}); + continue; + case CharT{'n'}: + AppendEscapedLiteral(&out, CharT{'\n'}); + continue; + case CharT{'t'}: + AppendEscapedLiteral(&out, CharT{'\t'}); + continue; + case CharT{'Q'}: + // Formatting a duration's %Q does not consistently apply the numeric locale. + AppendLocalizedField(&out, FormatArgument::TimeOfDayCount); + continue; + case CharT{'q'}: + if (use_microseconds_suffix) { + // Some standard libraries use "us"; Arrow uses the micro sign. + if constexpr (std::is_same_v<CharT, char>) { + AppendEscapedLiteral(&out, CharT{'\xC2'}); + AppendEscapedLiteral(&out, CharT{'\xB5'}); + } else { + AppendEscapedLiteral(&out, static_cast<CharT>(0xB5)); + } + AppendEscapedLiteral(&out, CharT{'s'}); + } else { + AppendChronoField(&out, FormatArgument::TimeOfDay, specifier); + } + continue; + default: + break; + } + } + +# if defined(__GLIBCXX__) + if (modifier == CharT{'O'} && specifier == CharT{'V'}) { + // libstdc++ does not yet accept %OV; use its equivalent base representation. + AppendChronoField(&out, FormatArgument::ZonedTime, specifier); + continue; + } +# endif + + if (IsSupportedStrftimeSpecifier(modifier, specifier)) { + AppendChronoField(&out, FormatArgument::ZonedTime, specifier, modifier); + } else { + AppendEscapedLiteral(&out, CharT{'%'}); + if (modifier != CharT{}) AppendEscapedLiteral(&out, modifier); + AppendEscapedLiteral(&out, specifier); + } + } + return out; +} + +} // namespace detail + +// Convert Arrow's strftime syntax to C++20 replacement fields. Literal braces and +// unsupported directives remain literal, and %Q/%q use local time of day. template <typename CharT, typename Traits, typename Duration, typename TimeZonePtr> std::basic_ostream<CharT, Traits>& to_stream( std::basic_ostream<CharT, Traits>& os, const CharT* fmt, const std::chrono::zoned_time<Duration, TimeZonePtr>& zt) { - std::vformat_to(std::ostreambuf_iterator<CharT>(os), std::string("{:") + fmt + "}", - std::make_format_args(zt)); + static_assert(std::is_same_v<CharT, char> || std::is_same_v<CharT, wchar_t>); + using Precision = typename std::chrono::zoned_time<Duration, TimeZonePtr>::duration; + const auto standard_format = detail::ToChronoFormat( + fmt, std::ratio_equal_v<typename Precision::period, std::micro>); + const auto local_time = zt.get_local_time(); + const auto local_day = std::chrono::floor<std::chrono::days>(local_time); + const auto time_of_day = local_time - local_day; + const auto time_of_day_count = time_of_day.count(); + + std::basic_string<CharT> formatted; + if constexpr (std::is_same_v<CharT, char>) { + formatted = std::vformat(os.getloc(), standard_format, + std::make_format_args(zt, time_of_day, time_of_day_count)); + } else { + formatted = std::vformat(os.getloc(), standard_format, + std::make_wformat_args(zt, time_of_day, time_of_day_count)); + } + os.write(formatted.data(), static_cast<std::streamsize>(formatted.size())); return os; } -// Format a duration using strftime-like format specifiers -// Converts "%H%M" style to C++20's "{:%H%M}" style and uses std::vformat -template <typename Duration> -std::string format(const char* fmt, const Duration& d) { - return std::vformat(std::string("{:") + fmt + "}", std::make_format_args(d)); +// Format a duration or time point using strftime-like format specifiers. +// Converts "%H%M" style to C++20's "{:L%H%M}" style and uses std::vformat. +template <typename Temporal> +std::string format(const char* fmt, const Temporal& value) { Review Comment: Does this implicitly call the `to_stream` above? If not, why doesn't it do the same `ToChronoFormat call? ########## cpp/src/arrow/util/chrono_internal.h: ########## @@ -151,22 +127,172 @@ inline const time_zone* locate_zone(std::string_view tz_name) { inline const time_zone* current_zone() { return std::chrono::current_zone(); } -// Formatting support - streams directly using C++20 std::vformat_to -// Provides: direct streaming, stream state preservation, chaining, rich format specifiers +namespace detail { + +// Argument positions passed to std::vformat by to_stream below. +enum class FormatArgument : char { + ZonedTime = '0', + TimeOfDay = '1', + TimeOfDayCount = '2', +}; + +template <typename CharT> +void AppendEscapedLiteral(std::basic_string<CharT>* out, CharT value) { + out->push_back(value); + if (value == CharT{'{'} || value == CharT{'}'}) { + out->push_back(value); + } +} + +// These are the directives accepted by Arrow's existing strftime syntax. Treat +// all others as literals to preserve compatibility. +template <typename CharT> +bool IsSupportedStrftimeSpecifier(CharT modifier, CharT specifier) { + const auto contains = [specifier](const char* candidates) { + for (; *candidates != '\0'; ++candidates) { + if (specifier == static_cast<CharT>(*candidates)) return true; + } + return false; + }; + if (modifier == CharT{}) { + return contains("aAbBhcCxdeDFgGHIjmMprRSTuUVWwXyYzZ"); + } + if (modifier == CharT{'E'}) { + return contains("cCxXyYz"); + } + if (modifier == CharT{'O'}) { + return contains("deHImMSuUVwWyz"); + } + return false; +} + +template <typename CharT> +void AppendChronoField(std::basic_string<CharT>* out, FormatArgument argument, + CharT specifier, CharT modifier = CharT{}) { + *out += {CharT{'{'}, static_cast<CharT>(argument), CharT{':'}, CharT{'L'}, CharT{'%'}}; + if (modifier != CharT{}) out->push_back(modifier); + *out += {specifier, CharT{'}'}}; +} + +template <typename CharT> +void AppendLocalizedField(std::basic_string<CharT>* out, FormatArgument argument) { + *out += {CharT{'{'}, static_cast<CharT>(argument), CharT{':'}, CharT{'L'}, CharT{'}'}}; +} + +template <typename CharT> +std::basic_string<CharT> ToChronoFormat(const CharT* fmt, bool use_microseconds_suffix) { + std::basic_string<CharT> out; + while (*fmt != CharT{}) { + if (*fmt != CharT{'%'}) { + AppendEscapedLiteral(&out, *fmt++); + continue; + } + + ++fmt; + if (*fmt == CharT{}) { + AppendEscapedLiteral(&out, CharT{'%'}); + break; + } + + CharT modifier{}; + if (*fmt == CharT{'E'} || *fmt == CharT{'O'}) { + modifier = *fmt++; + if (*fmt == CharT{}) { + AppendEscapedLiteral(&out, CharT{'%'}); + AppendEscapedLiteral(&out, modifier); + break; + } + } + const CharT specifier = *fmt++; + + if (modifier == CharT{}) { + switch (specifier) { + case CharT{'%'}: + AppendEscapedLiteral(&out, CharT{'%'}); + continue; + case CharT{'n'}: + AppendEscapedLiteral(&out, CharT{'\n'}); + continue; + case CharT{'t'}: + AppendEscapedLiteral(&out, CharT{'\t'}); + continue; + case CharT{'Q'}: + // Formatting a duration's %Q does not consistently apply the numeric locale. + AppendLocalizedField(&out, FormatArgument::TimeOfDayCount); + continue; + case CharT{'q'}: + if (use_microseconds_suffix) { + // Some standard libraries use "us"; Arrow uses the micro sign. + if constexpr (std::is_same_v<CharT, char>) { + AppendEscapedLiteral(&out, CharT{'\xC2'}); + AppendEscapedLiteral(&out, CharT{'\xB5'}); + } else { + AppendEscapedLiteral(&out, static_cast<CharT>(0xB5)); + } + AppendEscapedLiteral(&out, CharT{'s'}); + } else { + AppendChronoField(&out, FormatArgument::TimeOfDay, specifier); + } + continue; + default: + break; + } + } + +# if defined(__GLIBCXX__) + if (modifier == CharT{'O'} && specifier == CharT{'V'}) { + // libstdc++ does not yet accept %OV; use its equivalent base representation. + AppendChronoField(&out, FormatArgument::ZonedTime, specifier); + continue; + } +# endif + + if (IsSupportedStrftimeSpecifier(modifier, specifier)) { + AppendChronoField(&out, FormatArgument::ZonedTime, specifier, modifier); + } else { + AppendEscapedLiteral(&out, CharT{'%'}); + if (modifier != CharT{}) AppendEscapedLiteral(&out, modifier); + AppendEscapedLiteral(&out, specifier); + } + } + return out; +} + +} // namespace detail + +// Convert Arrow's strftime syntax to C++20 replacement fields. Literal braces and +// unsupported directives remain literal, and %Q/%q use local time of day. template <typename CharT, typename Traits, typename Duration, typename TimeZonePtr> std::basic_ostream<CharT, Traits>& to_stream( std::basic_ostream<CharT, Traits>& os, const CharT* fmt, const std::chrono::zoned_time<Duration, TimeZonePtr>& zt) { - std::vformat_to(std::ostreambuf_iterator<CharT>(os), std::string("{:") + fmt + "}", - std::make_format_args(zt)); + static_assert(std::is_same_v<CharT, char> || std::is_same_v<CharT, wchar_t>); + using Precision = typename std::chrono::zoned_time<Duration, TimeZonePtr>::duration; + const auto standard_format = detail::ToChronoFormat( + fmt, std::ratio_equal_v<typename Precision::period, std::micro>); + const auto local_time = zt.get_local_time(); + const auto local_day = std::chrono::floor<std::chrono::days>(local_time); + const auto time_of_day = local_time - local_day; + const auto time_of_day_count = time_of_day.count(); + + std::basic_string<CharT> formatted; + if constexpr (std::is_same_v<CharT, char>) { + formatted = std::vformat(os.getloc(), standard_format, Review Comment: Why not reuse the `std::vformat_to(std::ostreambuf_iterator<CharT>(os)` pattern that was in the previous version of this function? ########## cpp/src/arrow/vendored/datetime_ios.mm: ########## @@ -0,0 +1,25 @@ +// 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. + +// Evaluate automatic backend selection only when the build has not selected one. +#ifndef ARROW_USE_STD_CHRONO Review Comment: Why the guard? Just include the file and it won't do anything if `ARROW_USE_STD_CHRONO` is defined, right? ########## cpp/src/arrow/vendored/datetime.cpp: ########## @@ -15,5 +15,12 @@ // specific language governing permissions and limitations // under the License. -#include "datetime/visibility.h" -#include "datetime/tz.cpp" +#include "arrow/util/chrono_config_internal.h" + +// Keep backend selection identical to the callers, including in Gandiva tests. +// Standard-library builds must not compile a second timezone implementation. +#if !ARROW_USE_STD_CHRONO +# include "datetime/visibility.h" + Review Comment: Remove empty line? ########## cpp/src/arrow/util/chrono_internal.h: ########## @@ -151,22 +127,172 @@ inline const time_zone* locate_zone(std::string_view tz_name) { inline const time_zone* current_zone() { return std::chrono::current_zone(); } -// Formatting support - streams directly using C++20 std::vformat_to -// Provides: direct streaming, stream state preservation, chaining, rich format specifiers +namespace detail { + +// Argument positions passed to std::vformat by to_stream below. +enum class FormatArgument : char { + ZonedTime = '0', + TimeOfDay = '1', + TimeOfDayCount = '2', +}; + +template <typename CharT> +void AppendEscapedLiteral(std::basic_string<CharT>* out, CharT value) { + out->push_back(value); + if (value == CharT{'{'} || value == CharT{'}'}) { + out->push_back(value); + } +} + +// These are the directives accepted by Arrow's existing strftime syntax. Treat +// all others as literals to preserve compatibility. +template <typename CharT> +bool IsSupportedStrftimeSpecifier(CharT modifier, CharT specifier) { + const auto contains = [specifier](const char* candidates) { + for (; *candidates != '\0'; ++candidates) { + if (specifier == static_cast<CharT>(*candidates)) return true; + } + return false; + }; + if (modifier == CharT{}) { + return contains("aAbBhcCxdeDFgGHIjmMprRSTuUVWwXyYzZ"); + } + if (modifier == CharT{'E'}) { + return contains("cCxXyYz"); + } + if (modifier == CharT{'O'}) { + return contains("deHImMSuUVwWyz"); + } + return false; +} + +template <typename CharT> +void AppendChronoField(std::basic_string<CharT>* out, FormatArgument argument, + CharT specifier, CharT modifier = CharT{}) { + *out += {CharT{'{'}, static_cast<CharT>(argument), CharT{':'}, CharT{'L'}, CharT{'%'}}; + if (modifier != CharT{}) out->push_back(modifier); + *out += {specifier, CharT{'}'}}; +} + +template <typename CharT> +void AppendLocalizedField(std::basic_string<CharT>* out, FormatArgument argument) { + *out += {CharT{'{'}, static_cast<CharT>(argument), CharT{':'}, CharT{'L'}, CharT{'}'}}; +} + +template <typename CharT> +std::basic_string<CharT> ToChronoFormat(const CharT* fmt, bool use_microseconds_suffix) { + std::basic_string<CharT> out; + while (*fmt != CharT{}) { + if (*fmt != CharT{'%'}) { + AppendEscapedLiteral(&out, *fmt++); + continue; + } + + ++fmt; + if (*fmt == CharT{}) { + AppendEscapedLiteral(&out, CharT{'%'}); + break; + } + + CharT modifier{}; + if (*fmt == CharT{'E'} || *fmt == CharT{'O'}) { + modifier = *fmt++; + if (*fmt == CharT{}) { + AppendEscapedLiteral(&out, CharT{'%'}); + AppendEscapedLiteral(&out, modifier); + break; + } + } + const CharT specifier = *fmt++; + + if (modifier == CharT{}) { + switch (specifier) { + case CharT{'%'}: + AppendEscapedLiteral(&out, CharT{'%'}); + continue; + case CharT{'n'}: + AppendEscapedLiteral(&out, CharT{'\n'}); + continue; + case CharT{'t'}: + AppendEscapedLiteral(&out, CharT{'\t'}); + continue; + case CharT{'Q'}: + // Formatting a duration's %Q does not consistently apply the numeric locale. + AppendLocalizedField(&out, FormatArgument::TimeOfDayCount); + continue; + case CharT{'q'}: + if (use_microseconds_suffix) { + // Some standard libraries use "us"; Arrow uses the micro sign. + if constexpr (std::is_same_v<CharT, char>) { + AppendEscapedLiteral(&out, CharT{'\xC2'}); + AppendEscapedLiteral(&out, CharT{'\xB5'}); + } else { + AppendEscapedLiteral(&out, static_cast<CharT>(0xB5)); + } + AppendEscapedLiteral(&out, CharT{'s'}); + } else { + AppendChronoField(&out, FormatArgument::TimeOfDay, specifier); + } + continue; + default: + break; + } + } + +# if defined(__GLIBCXX__) + if (modifier == CharT{'O'} && specifier == CharT{'V'}) { + // libstdc++ does not yet accept %OV; use its equivalent base representation. + AppendChronoField(&out, FormatArgument::ZonedTime, specifier); + continue; + } +# endif + + if (IsSupportedStrftimeSpecifier(modifier, specifier)) { + AppendChronoField(&out, FormatArgument::ZonedTime, specifier, modifier); + } else { + AppendEscapedLiteral(&out, CharT{'%'}); + if (modifier != CharT{}) AppendEscapedLiteral(&out, modifier); + AppendEscapedLiteral(&out, specifier); + } + } + return out; +} + +} // namespace detail + +// Convert Arrow's strftime syntax to C++20 replacement fields. Literal braces and +// unsupported directives remain literal, and %Q/%q use local time of day. template <typename CharT, typename Traits, typename Duration, typename TimeZonePtr> std::basic_ostream<CharT, Traits>& to_stream( std::basic_ostream<CharT, Traits>& os, const CharT* fmt, const std::chrono::zoned_time<Duration, TimeZonePtr>& zt) { - std::vformat_to(std::ostreambuf_iterator<CharT>(os), std::string("{:") + fmt + "}", - std::make_format_args(zt)); + static_assert(std::is_same_v<CharT, char> || std::is_same_v<CharT, wchar_t>); + using Precision = typename std::chrono::zoned_time<Duration, TimeZonePtr>::duration; + const auto standard_format = detail::ToChronoFormat( + fmt, std::ratio_equal_v<typename Precision::period, std::micro>); Review Comment: Wait, are we calling `ToChronoFormat` for each single datetime value that's being formatted? This seems wasteful. ########## cpp/src/arrow/util/chrono_config_internal.h: ########## @@ -0,0 +1,56 @@ +// 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. + +#pragma once + +#include <chrono> + +// Share backend selection with the vendored implementation without including its +// headers. datetime.h undefines macros needed when compiling the implementation. +// +// On Windows, MSVC's standard library uses the system timezone database, while +// libstdc++ reads tzdata files (using TZDIR). Libraries without the C++20 timezone +// APIs, including older libc++, still require the vendored date library. +// +// Use the standard backend by default. Builds may explicitly define +// ARROW_USE_STD_CHRONO to 0 or 1 when they need to select a backend. +// +// Automatically disable the default for libraries without the C++20 timezone APIs. +// On non-Windows, older libstdc++ versions also need the fallback because of +// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116110 (fully fixed in GCC 16.2). +// Check library macros, not __GNUC__, so Clang using libstdc++ agrees with GCC. Review Comment: This sentence is of little value. Obviously we use libstdc++ symbols when checking for libstdc++... ########## cpp/src/arrow/util/logger_test.cc: ########## @@ -23,8 +23,9 @@ #include "arrow/testing/gtest_util.h" #include "arrow/util/logger.h" -// Emit log via the default logger -#define DO_LOG(LEVEL, ...) ARROW_LOGGER_CALL("", LEVEL, __VA_ARGS__) +// Emit log via the default logger. Token-paste here to prevent Windows' ERROR +// macro from expanding before the logger macro is selected. Review Comment: Why did this suddenly start to occur? ########## cpp/src/arrow/util/CMakeLists.txt: ########## @@ -22,6 +22,11 @@ # Headers: top level arrow_install_all_headers("arrow/util") +# The automatic rule excludes internal headers, but these are dependencies of +# the installed formatting.h and value_parsing.h headers. Review Comment: Then why make them internal? -- 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]
