Skip to content

GH-51215: [C++] Migrate remaining datetime consumers to the chrono shim #51216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rok wants to merge 12 commits into
base: main
Choose a base branch
from
Open

GH-51215: [C++] Migrate remaining datetime consumers to the chrono shim #51216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6998bb4
GH-51215: [C++] Migrate datetime consumers to the chrono shim
rok Sep 7, 2026
7e450a6
GH-51215: [C++] Preserve strftime behavior with standard chrono
rok Sep 8, 2026
cc92cd3
GH-51215: [CI] Add GCC 16 experimental C++ task
rok Sep 8, 2026
c97bdd5
GH-51215: [C++] Address chrono shim review feedback
rok Sep 17, 2026
2ab86d6
GH-51215: [C++] Move strftime formatting out of the chrono shim
rok Sep 17, 2026
5781ee3
Fold StrftimeFormatter into TimestampFormatter
rok Sep 17, 2026
e8de86c
compatible directives should share chrono field
rok Sep 17, 2026
dfb9ed7
restore format as const
rok Sep 17, 2026
48be9cc
GH-51215: [C++][R][Python] Address chrono backend review concerns
rok Sep 30, 2026
14c50d8
update R comment
rok Sep 30, 2026
6e9782e
GH-51215: [C++] Avoid overflow in timestamp time-of-day formatting
rok Sep 30, 2026
fd2bb12
comment
rok Sep 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cpp/src/arrow/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,8 @@ set(ARROW_VENDORED_SRCS
vendored/uriparser/UriResolve.c
vendored/uriparser/UriShorten.c)
if(APPLE)
list(APPEND ARROW_VENDORED_SRCS vendored/datetime/ios.mm)
# This wrapper excludes the iOS timezone implementation from standard-backend builds.
list(APPEND ARROW_VENDORED_SRCS vendored/datetime_ios.mm)
endif()
set_source_files_properties(vendored/datetime.cpp PROPERTIES SKIP_UNITY_BUILD_INCLUSION
ON)
Expand Down
30 changes: 15 additions & 15 deletions cpp/src/arrow/array/diff.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,19 @@
#include "arrow/type_traits.h"
#include "arrow/util/bit_util.h"
#include "arrow/util/checked_cast.h"
#include "arrow/util/chrono_internal.h"
#include "arrow/util/float16.h"
#include "arrow/util/logging_internal.h"
#include "arrow/util/range.h"
#include "arrow/util/ree_util.h"
#include "arrow/util/string.h"
#include "arrow/util/unreachable.h"
#include "arrow/vendored/datetime.h"
#include "arrow/visit_type_inline.h"

namespace arrow {

namespace chrono = internal::chrono;

using internal::checked_cast;
using internal::checked_pointer_cast;
using internal::MakeLazyRange;
Expand Down Expand Up @@ -631,14 +633,13 @@ class MakeFormatterImpl {
template <typename T>
enable_if_date<T, Status> Visit(const T&) {
using unit = typename std::conditional<std::is_same<T, Date32Type>::value,
arrow_vendored::date::days,
std::chrono::milliseconds>::type;
chrono::days, std::chrono::milliseconds>::type;

static arrow_vendored::date::sys_days epoch{arrow_vendored::date::jan / 1 / 1970};
static chrono::sys_days epoch{chrono::jan / 1 / 1970};

impl_ = [](const Array& array, int64_t index, std::ostream* os) {
unit value(checked_cast<const NumericArray<T>&>(array).Value(index));
*os << arrow_vendored::date::format("%F", value + epoch);
*os << chrono::format("%F", value + epoch);
};
return Status::OK();
}
Expand Down Expand Up @@ -854,42 +855,41 @@ class MakeFormatterImpl {
auto value = checked_cast<const NumericArray<T>&>(array).Value(index);
// Using unqualified `format` directly would produce ambiguous
// lookup because of `std::format` (ARROW-15520).
namespace avd = arrow_vendored::date;
using std::chrono::nanoseconds;
using std::chrono::microseconds;
using std::chrono::milliseconds;
using std::chrono::seconds;
if (AddEpoch) {
static avd::sys_days epoch{avd::jan / 1 / 1970};
static chrono::sys_days epoch{chrono::jan / 1 / 1970};

switch (unit) {
case TimeUnit::NANO:
*os << avd::format(fmt, static_cast<nanoseconds>(value) + epoch);
*os << chrono::format(fmt, static_cast<nanoseconds>(value) + epoch);
break;
case TimeUnit::MICRO:
*os << avd::format(fmt, static_cast<microseconds>(value) + epoch);
*os << chrono::format(fmt, static_cast<microseconds>(value) + epoch);
break;
case TimeUnit::MILLI:
*os << avd::format(fmt, static_cast<milliseconds>(value) + epoch);
*os << chrono::format(fmt, static_cast<milliseconds>(value) + epoch);
break;
case TimeUnit::SECOND:
*os << avd::format(fmt, static_cast<seconds>(value) + epoch);
*os << chrono::format(fmt, static_cast<seconds>(value) + epoch);
break;
}
return;
}
switch (unit) {
case TimeUnit::NANO:
*os << avd::format(fmt, static_cast<nanoseconds>(value));
*os << chrono::format(fmt, static_cast<nanoseconds>(value));
break;
case TimeUnit::MICRO:
*os << avd::format(fmt, static_cast<microseconds>(value));
*os << chrono::format(fmt, static_cast<microseconds>(value));
break;
case TimeUnit::MILLI:
*os << avd::format(fmt, static_cast<milliseconds>(value));
*os << chrono::format(fmt, static_cast<milliseconds>(value));
break;
case TimeUnit::SECOND:
*os << avd::format(fmt, static_cast<seconds>(value));
*os << chrono::format(fmt, static_cast<seconds>(value));
break;
}
};
Expand Down
3 changes: 1 addition & 2 deletions cpp/src/arrow/compute/kernels/scalar_cast_temporal.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,8 +462,7 @@ struct ParseDate {
using value_type = typename DateType::c_type;

using duration_type =
typename std::conditional<std::is_same<DateType, Date32Type>::value,
arrow_vendored::date::days,
typename std::conditional<std::is_same<DateType, Date32Type>::value, chrono::days,
std::chrono::milliseconds>::type;

template <typename OutValue, typename Arg0Value>
Expand Down
117 changes: 117 additions & 0 deletions cpp/src/arrow/compute/kernels/scalar_temporal_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,15 @@
// specific language governing permissions and limitations
// under the License.

#include <limits>
#include <sstream>
#include <tuple>

#include <gtest/gtest.h>

#include "arrow/compute/api_scalar.h"
#include "arrow/compute/cast.h"
#include "arrow/compute/kernels/temporal_internal.h"
#include "arrow/compute/kernels/test_util_internal.h"
#include "arrow/testing/gtest_util.h"
#include "arrow/testing/matchers.h"
Expand Down Expand Up @@ -2003,6 +2006,120 @@ TEST_F(ScalarTemporalTest, TestAssumeTimezoneNonexistent) {
&options_earliest);
}

#if ARROW_USE_STD_CHRONO

@pitrou pitrou Sep 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these tests are not compute-specific, can we move them to a more appropriate place? For example arrow/util/time_test.cc

TEST(TimestampFormatterTest, CoalesceChronoFields) {

@pitrou pitrou Sep 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a comment explaining what this is about?

using internal::detail::ToChronoFormat;
EXPECT_EQ(ToChronoFormat(StrftimeOptions::kDefaultFormat, false),
"{0:L%Y-%m-%dT%H:%M:%S}");
EXPECT_EQ(ToChronoFormat("%Y%m%d %H%M%S %Ez %Z", false), "{0:L%Y%m%d %H%M%S %Ez %Z}");
EXPECT_EQ(ToChronoFormat("%Y%n%t%m", false), "{0:L%Y\n\t%m}");
EXPECT_EQ(ToChronoFormat("%Y{%m}%d", false), "{0:L%Y}{{{0:L%m}}}{0:L%d}");
EXPECT_EQ(ToChronoFormat("%Y%%%m%J%d%E", false), "{0:L%Y}%{0:L%m}%J{0:L%d}%E");
EXPECT_EQ(ToChronoFormat("%Y %Q %m %q %d", false),
"{0:L%Y }{2:L} {0:L%m }{1:L%q} {0:L%d}");
EXPECT_EQ(ToChronoFormat("%Y %Q %m %q %d", true),
"{0:L%Y }{2:L} {0:L%m }\xC2\xB5s {0:L%d}");
}
#endif

TEST(TimestampFormatterTest, ReuseFormatter) {

@pitrou pitrou Sep 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, explain why this is useful?

const arrow::internal::OffsetZone zone{std::chrono::minutes{60}};
internal::TimestampFormatter<std::chrono::microseconds> formatter{
"{%F %T} %Q %q %J", zone, std::locale::classic()};
for (const auto& [count, expected] :
{std::pair{1, "{1970-01-01 01:00:00.000001} 3600000001 \xC2\xB5s %J"},
std::pair{-1, "{1970-01-01 00:59:59.999999} 3599999999 \xC2\xB5s %J"}}) {
ASSERT_OK_AND_ASSIGN(auto result, formatter(count));
EXPECT_EQ(result, expected);
}
}

TEST(TimestampFormatterTest, NanosecondRangeLimits) {

@pitrou pitrou Sep 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we test for overflow somewhere?

const arrow::internal::OffsetZone zone{std::chrono::minutes{0}};
internal::TimestampFormatter<std::chrono::nanoseconds> formatter{
"%F %T %Q %q", zone, std::locale::classic()};
for (const auto& [count, expected] :
{std::pair{std::numeric_limits<int64_t>::min() + 1000000000,
"1677-09-21 00:12:44.145224192 764145224192 ns"},
std::pair{int64_t{-86400000000000}, "1969-12-31 00:00:00.000000000 0 ns"},
std::pair{int64_t{-1}, "1969-12-31 23:59:59.999999999 86399999999999 ns"},
std::pair{int64_t{0}, "1970-01-01 00:00:00.000000000 0 ns"},
std::pair{int64_t{1}, "1970-01-01 00:00:00.000000001 1 ns"},
std::pair{std::numeric_limits<int64_t>::max(),
"2262-04-11 23:47:16.854775807 85636854775807 ns"}}) {
SCOPED_TRACE(count);
ASSERT_OK_AND_ASSIGN(auto result, formatter(count));
EXPECT_EQ(result, expected);
}

// Formatting calendar fields at the exact lower bound can overflow within
// standard-library formatters. Test the time-of-day directives independently.
internal::TimestampFormatter<std::chrono::nanoseconds> count_formatter{
"%Q %q", zone, std::locale::classic()};
ASSERT_OK_AND_ASSIGN(auto result, count_formatter(std::numeric_limits<int64_t>::min()));
EXPECT_EQ(result, "763145224192 ns");
}

TEST(TimestampFormatterTest, StreamState) {

@pitrou pitrou Sep 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for exactly? Is it useful? if so, add a comment?

internal::TimestampFormatter<std::chrono::seconds> formatter{
"%F %T", arrow::internal::OffsetZone{std::chrono::minutes{0}},
std::locale::classic()};
auto& out = formatter.bufstream;
out << std::hex << std::showbase;
out.precision(3);
out.width(30);
out.fill('*');
Comment on lines +2068 to +2071

@pitrou pitrou Sep 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why on Earth are we testing this? Is it a supported usage of these internal APIs?

const auto flags = out.flags();
ASSERT_OK_AND_ASSIGN(auto result, formatter(0));
EXPECT_EQ(result, "1970-01-01 00:00:00");
EXPECT_EQ(out.flags(), flags);
EXPECT_EQ(out.precision(), 3);
EXPECT_EQ(out.width(), 30);
EXPECT_EQ(out.fill(), '*');

// A streambuf with no put area rejects every write.
class FailingBuffer : public std::streambuf {
} buffer;
Comment on lines +2080 to +2082

@pitrou pitrou Sep 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WHY?

auto& stream = static_cast<std::ostream&>(out);
auto* original = stream.rdbuf(&buffer);
EXPECT_RAISES_WITH_MESSAGE_THAT(
Invalid, testing::HasSubstr("Failed formatting timestamp"), formatter(0));
stream.rdbuf(original);
ASSERT_OK_AND_ASSIGN(result, formatter(0));
EXPECT_EQ(result, "1970-01-01 00:00:00");
}

TEST_F(ScalarTemporalTest, StrftimeFormatSyntax) {
const auto type = timestamp(TimeUnit::MILLI, "UTC");
const char* input = R"(["1970-01-01T00:00:00.123", null])";
for (const auto& [format, expected] :
{std::pair{"", R"(["", null])"},
std::pair{"literal {%Y}", R"(["literal {1970}", null])"},
std::pair{"unmatched }%Y{", R"(["unmatched }1970{", null])"},
std::pair{"%Y}", R"(["1970}", null])"},
std::pair{"%Y{%m}%d", R"(["1970{01}01", null])"},
std::pair{"%Y%%%m%J%d%E", R"(["1970%01%J01%E", null])"},
std::pair{"%Y%n%t%m", R"(["1970\n\t01", null])"},
std::pair{"%Y %Q %m %q %d", R"(["1970 123 01 ms 01", null])"},
std::pair{"%Q %q %J %z %Z", R"(["123 ms %J +0000 UTC", null])"},
std::pair{"%% %n%t %Ez %Oz %OV %EJ end%",
R"(["% \n\t +00:00 +00:00 01 %EJ end%", null])"}}) {
SCOPED_TRACE(format);
const auto options = StrftimeOptions(format);
CheckScalarUnary("strftime", type, input, utf8(), expected, &options);
}

for (const auto& [format, expected] :
{std::pair{"%Q %q", R"(["1 \u00b5s", null])"},
std::pair{"%Y %Q %m %q %d", R"(["1970 1 01 \u00b5s 01", null])"}}) {
SCOPED_TRACE(format);
const auto options = StrftimeOptions(format);
CheckScalarUnary("strftime", timestamp(TimeUnit::MICRO, "UTC"),
R"(["1970-01-01T00:00:00.000001", null])", utf8(), expected,
&options);
}
}

TEST_F(ScalarTemporalTest, StrftimeOffsetTimezone) {
auto options_ymdhms = StrftimeOptions("%Y-%m-%dT%H:%M:%S");

Expand Down
Loading
Loading