pitrou commented on code in PR #51216:
URL: https://github.com/apache/arrow/pull/51216#discussion_r4144470552
##########
cpp/src/arrow/compute/kernels/scalar_temporal_test.cc:
##########
@@ -2003,6 +2006,120 @@ TEST_F(ScalarTemporalTest,
TestAssumeTimezoneNonexistent) {
&options_earliest);
}
+#if ARROW_USE_STD_CHRONO
+TEST(TimestampFormatterTest, CoalesceChronoFields) {
+ 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) {
+ 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) {
+ 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) {
Review Comment:
What is this for exactly? Is it useful? if so, add a comment?
##########
cpp/src/arrow/compute/kernels/scalar_temporal_test.cc:
##########
@@ -2003,6 +2006,120 @@ TEST_F(ScalarTemporalTest,
TestAssumeTimezoneNonexistent) {
&options_earliest);
}
+#if ARROW_USE_STD_CHRONO
+TEST(TimestampFormatterTest, CoalesceChronoFields) {
+ 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) {
+ 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) {
Review Comment:
Do we test for overflow somewhere?
##########
cpp/src/arrow/compute/kernels/scalar_temporal_test.cc:
##########
@@ -2003,6 +2006,120 @@ TEST_F(ScalarTemporalTest,
TestAssumeTimezoneNonexistent) {
&options_earliest);
}
+#if ARROW_USE_STD_CHRONO
+TEST(TimestampFormatterTest, CoalesceChronoFields) {
+ 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) {
Review Comment:
Same, explain why this is useful?
##########
cpp/src/arrow/compute/kernels/scalar_temporal_test.cc:
##########
@@ -2003,6 +2006,120 @@ TEST_F(ScalarTemporalTest,
TestAssumeTimezoneNonexistent) {
&options_earliest);
}
+#if ARROW_USE_STD_CHRONO
+TEST(TimestampFormatterTest, CoalesceChronoFields) {
+ 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) {
+ 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) {
+ 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) {
+ 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('*');
Review Comment:
Why on Earth are we testing this? Is it a supported usage of these internal
APIs?
##########
cpp/src/arrow/compute/kernels/scalar_temporal_test.cc:
##########
@@ -2003,6 +2006,120 @@ TEST_F(ScalarTemporalTest,
TestAssumeTimezoneNonexistent) {
&options_earliest);
}
+#if ARROW_USE_STD_CHRONO
+TEST(TimestampFormatterTest, CoalesceChronoFields) {
Review Comment:
Could you add a comment explaining what this is about?
##########
cpp/src/arrow/compute/kernels/temporal_internal.h:
##########
@@ -163,15 +174,174 @@ struct ZonedLocalizer {
local_days ConvertDays(sys_days d) const { return
local_days(year_month_day(d)); }
};
+#if ARROW_USE_STD_CHRONO
+namespace detail {
Review Comment:
There are probably some things here that are not performance-critical to put
in a `.h` file, for example `ToChronoFormat`. Can we move them to a `.cc` and
perhaps the reduce the header inclusions here?
##########
cpp/src/arrow/compute/kernels/temporal_internal.h:
##########
@@ -182,7 +352,29 @@ struct TimestampFormatter {
const auto timepoint = sys_time<Duration>(Duration{arg});
auto format_zoned_time = [&](auto&& zt) {
try {
- chrono::to_stream(bufstream, format.c_str(), zt);
+#if ARROW_USE_STD_CHRONO
+ // %Q/%q refer to local time of day rather than elapsed time since the
epoch.
+ const auto local_time = zt.get_local_time();
+ // Truncate toward zero, as in StringFormatter<TimestampType>, so
midnight
+ // remains representable even near the lower bound of nanosecond
timestamps.
+ const auto local_day =
+ std::chrono::time_point_cast<std::chrono::days>(local_time);
+ const auto time_of_day = local_day <= local_time
+ ? local_time - local_day
+ : std::chrono::days{1} - (local_day -
local_time);
+ const auto time_of_day_count = time_of_day.count();
+ const std::ostream::sentry sentry(bufstream);
Review Comment:
Hmm, what is this for/how this snippet work? Can you add explanatory
comments?
##########
cpp/src/arrow/compute/kernels/temporal_internal.h:
##########
@@ -163,15 +174,174 @@ struct ZonedLocalizer {
local_days ConvertDays(sys_days d) const { return
local_days(year_month_day(d)); }
};
+#if ARROW_USE_STD_CHRONO
+namespace detail {
+
+// Argument positions passed to std::vformat_to by TimestampFormatter below.
+enum class FormatArgument : char {
+ ZonedTime = '0',
+ TimeOfDay = '1',
+ TimeOfDayCount = '2',
+};
+
+inline void AppendEscapedLiteral(std::string* out, char value) {
+ out->push_back(value);
+ if (value == '{' || value == '}') {
+ out->push_back(value);
+ }
+}
+
+// These are the directives accepted by Arrow's existing strftime syntax. Treat
+// all others as literals to preserve compatibility.
+inline bool IsSupportedStrftimeSpecifier(char modifier, char specifier) {
+ const auto contains = [specifier](std::string_view candidates) {
+ return candidates.find(specifier) != std::string_view::npos;
+ };
+ if (modifier == '\0') {
+ return contains("aAbBhcCxdeDFgGHIjmMprRSTuUVWwXyYzZ");
+ }
+ if (modifier == 'E') {
+ return contains("cCxXyYz");
+ }
+ if (modifier == 'O') {
+ return contains("deHImMSuUVwWyz");
+ }
+ return false;
+}
+
+inline void AppendChronoField(std::string* out, FormatArgument argument, char
specifier,
+ char modifier = '\0') {
+ *out += {'{', static_cast<char>(argument), ':', 'L', '%'};
+ if (modifier != '\0') out->push_back(modifier);
+ *out += {specifier, '}'};
+}
+
+inline void AppendLocalizedField(std::string* out, FormatArgument argument) {
+ *out += {'{', static_cast<char>(argument), ':', 'L', '}'};
+}
+
+inline std::string ToChronoFormat(const char* fmt, bool
use_microseconds_suffix) {
+ std::string out;
+ bool zoned_field_open = false;
+ const auto close_zoned_field = [&] {
+ if (zoned_field_open) {
+ out.push_back('}');
+ zoned_field_open = false;
+ }
+ };
+ const auto append_literal = [&](char value) {
+ // Braces and literal percent signs must stay outside chrono replacement
fields.
+ if (value == '{' || value == '}' || value == '%') close_zoned_field();
+ AppendEscapedLiteral(&out, value);
+ };
+ const auto append_zoned_directive = [&](char specifier, char modifier =
'\0') {
+ // Keep compatible directives and intervening literals in one field to
avoid
+ // repeating timezone lookup and calendar decomposition for every
directive.
+ if (!zoned_field_open) {
+ out += {'{', static_cast<char>(FormatArgument::ZonedTime), ':', 'L'};
+ zoned_field_open = true;
+ }
+ out.push_back('%');
+ if (modifier != '\0') out.push_back(modifier);
+ out.push_back(specifier);
+ };
+ while (*fmt != '\0') {
+ if (*fmt != '%') {
+ append_literal(*fmt++);
+ continue;
+ }
+
+ ++fmt;
+ if (*fmt == '\0') {
+ append_literal('%');
+ break;
+ }
+
+ char modifier = '\0';
+ if (*fmt == 'E' || *fmt == 'O') {
+ modifier = *fmt++;
+ if (*fmt == '\0') {
+ append_literal('%');
+ append_literal(modifier);
+ break;
+ }
+ }
+ const char specifier = *fmt++;
+
+ if (modifier == '\0') {
+ switch (specifier) {
+ case '%':
+ append_literal('%');
+ continue;
+ case 'n':
+ append_literal('\n');
+ continue;
+ case 't':
+ append_literal('\t');
+ continue;
+ case 'Q':
+ // Formatting a duration's %Q does not consistently apply the
numeric locale.
+ close_zoned_field();
+ AppendLocalizedField(&out, FormatArgument::TimeOfDayCount);
+ continue;
+ case 'q':
+ close_zoned_field();
+ if (use_microseconds_suffix) {
+ // Some standard libraries use "us"; Arrow uses the micro sign.
+ out += "\xC2\xB5s";
+ } else {
+ AppendChronoField(&out, FormatArgument::TimeOfDay, specifier);
+ }
+ continue;
+ default:
+ break;
+ }
+ }
+
+# if defined(__GLIBCXX__)
+ if (modifier == 'O' && specifier == 'V') {
+ // libstdc++ does not yet accept %OV; fall back to %V, losing any
+ // locale-specific alternative digits.
+ append_zoned_directive(specifier);
+ continue;
+ }
+# endif
+
+ if (IsSupportedStrftimeSpecifier(modifier, specifier)) {
+ append_zoned_directive(specifier, modifier);
+ } else {
+ append_literal('%');
+ if (modifier != '\0') append_literal(modifier);
+ append_literal(specifier);
+ }
+ }
+ close_zoned_field();
+ return out;
+}
+
+} // namespace detail
+#endif
+
template <typename Duration>
struct TimestampFormatter {
+ static std::string PrepareFormat(const std::string& format) {
Review Comment:
Can you make this method protected and move it towards the end of the struct
for clarity?
##########
cpp/src/arrow/compute/kernels/temporal_internal.h:
##########
@@ -163,15 +174,174 @@ struct ZonedLocalizer {
local_days ConvertDays(sys_days d) const { return
local_days(year_month_day(d)); }
};
+#if ARROW_USE_STD_CHRONO
+namespace detail {
+
+// Argument positions passed to std::vformat_to by TimestampFormatter below.
+enum class FormatArgument : char {
+ ZonedTime = '0',
+ TimeOfDay = '1',
+ TimeOfDayCount = '2',
+};
+
+inline void AppendEscapedLiteral(std::string* out, char value) {
+ out->push_back(value);
+ if (value == '{' || value == '}') {
+ out->push_back(value);
+ }
+}
+
+// These are the directives accepted by Arrow's existing strftime syntax. Treat
+// all others as literals to preserve compatibility.
+inline bool IsSupportedStrftimeSpecifier(char modifier, char specifier) {
+ const auto contains = [specifier](std::string_view candidates) {
+ return candidates.find(specifier) != std::string_view::npos;
+ };
+ if (modifier == '\0') {
+ return contains("aAbBhcCxdeDFgGHIjmMprRSTuUVWwXyYzZ");
+ }
+ if (modifier == 'E') {
+ return contains("cCxXyYz");
+ }
+ if (modifier == 'O') {
+ return contains("deHImMSuUVwWyz");
+ }
+ return false;
+}
+
+inline void AppendChronoField(std::string* out, FormatArgument argument, char
specifier,
+ char modifier = '\0') {
+ *out += {'{', static_cast<char>(argument), ':', 'L', '%'};
+ if (modifier != '\0') out->push_back(modifier);
+ *out += {specifier, '}'};
+}
+
+inline void AppendLocalizedField(std::string* out, FormatArgument argument) {
+ *out += {'{', static_cast<char>(argument), ':', 'L', '}'};
+}
+
+inline std::string ToChronoFormat(const char* fmt, bool
use_microseconds_suffix) {
+ std::string out;
+ bool zoned_field_open = false;
+ const auto close_zoned_field = [&] {
+ if (zoned_field_open) {
+ out.push_back('}');
+ zoned_field_open = false;
+ }
+ };
+ const auto append_literal = [&](char value) {
+ // Braces and literal percent signs must stay outside chrono replacement
fields.
+ if (value == '{' || value == '}' || value == '%') close_zoned_field();
+ AppendEscapedLiteral(&out, value);
+ };
+ const auto append_zoned_directive = [&](char specifier, char modifier =
'\0') {
+ // Keep compatible directives and intervening literals in one field to
avoid
+ // repeating timezone lookup and calendar decomposition for every
directive.
+ if (!zoned_field_open) {
+ out += {'{', static_cast<char>(FormatArgument::ZonedTime), ':', 'L'};
+ zoned_field_open = true;
+ }
+ out.push_back('%');
+ if (modifier != '\0') out.push_back(modifier);
+ out.push_back(specifier);
+ };
+ while (*fmt != '\0') {
+ if (*fmt != '%') {
+ append_literal(*fmt++);
+ continue;
+ }
+
+ ++fmt;
+ if (*fmt == '\0') {
+ append_literal('%');
+ break;
+ }
+
+ char modifier = '\0';
+ if (*fmt == 'E' || *fmt == 'O') {
+ modifier = *fmt++;
+ if (*fmt == '\0') {
+ append_literal('%');
+ append_literal(modifier);
+ break;
+ }
+ }
+ const char specifier = *fmt++;
+
+ if (modifier == '\0') {
+ switch (specifier) {
+ case '%':
+ append_literal('%');
+ continue;
+ case 'n':
+ append_literal('\n');
+ continue;
+ case 't':
+ append_literal('\t');
+ continue;
+ case 'Q':
+ // Formatting a duration's %Q does not consistently apply the
numeric locale.
+ close_zoned_field();
+ AppendLocalizedField(&out, FormatArgument::TimeOfDayCount);
+ continue;
+ case 'q':
+ close_zoned_field();
+ if (use_microseconds_suffix) {
+ // Some standard libraries use "us"; Arrow uses the micro sign.
+ out += "\xC2\xB5s";
+ } else {
+ AppendChronoField(&out, FormatArgument::TimeOfDay, specifier);
+ }
+ continue;
+ default:
+ break;
+ }
+ }
+
+# if defined(__GLIBCXX__)
+ if (modifier == 'O' && specifier == 'V') {
+ // libstdc++ does not yet accept %OV; fall back to %V, losing any
+ // locale-specific alternative digits.
+ append_zoned_directive(specifier);
+ continue;
+ }
+# endif
+
+ if (IsSupportedStrftimeSpecifier(modifier, specifier)) {
+ append_zoned_directive(specifier, modifier);
+ } else {
+ append_literal('%');
+ if (modifier != '\0') append_literal(modifier);
+ append_literal(specifier);
+ }
+ }
+ close_zoned_field();
+ return out;
+}
+
+} // namespace detail
+#endif
+
template <typename Duration>
struct TimestampFormatter {
+ static std::string PrepareFormat(const std::string& format) {
+#if ARROW_USE_STD_CHRONO
+ // Translate strftime syntax once, not for every timestamp.
+ using Precision = typename chrono::zoned_time<Duration>::duration;
+ return detail::ToChronoFormat(
+ format.c_str(), std::ratio_equal_v<typename Precision::period,
std::micro>);
Review Comment:
Please add parameter name when it's not obvious.
```suggestion
return detail::ToChronoFormat(
format.c_str(), /*xxx=*/ std::ratio_equal_v<typename
Precision::period, std::micro>);
```
##########
cpp/src/arrow/compute/kernels/scalar_temporal_test.cc:
##########
@@ -2003,6 +2006,120 @@ TEST_F(ScalarTemporalTest,
TestAssumeTimezoneNonexistent) {
&options_earliest);
}
+#if ARROW_USE_STD_CHRONO
+TEST(TimestampFormatterTest, CoalesceChronoFields) {
+ 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) {
+ 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) {
+ 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) {
+ 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('*');
+ 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;
Review Comment:
WHY?
##########
cpp/src/arrow/compute/kernels/temporal_internal.h:
##########
@@ -163,15 +174,174 @@ struct ZonedLocalizer {
local_days ConvertDays(sys_days d) const { return
local_days(year_month_day(d)); }
};
+#if ARROW_USE_STD_CHRONO
+namespace detail {
+
+// Argument positions passed to std::vformat_to by TimestampFormatter below.
+enum class FormatArgument : char {
+ ZonedTime = '0',
+ TimeOfDay = '1',
+ TimeOfDayCount = '2',
+};
+
+inline void AppendEscapedLiteral(std::string* out, char value) {
+ out->push_back(value);
+ if (value == '{' || value == '}') {
+ out->push_back(value);
+ }
+}
+
+// These are the directives accepted by Arrow's existing strftime syntax. Treat
+// all others as literals to preserve compatibility.
+inline bool IsSupportedStrftimeSpecifier(char modifier, char specifier) {
+ const auto contains = [specifier](std::string_view candidates) {
+ return candidates.find(specifier) != std::string_view::npos;
+ };
+ if (modifier == '\0') {
+ return contains("aAbBhcCxdeDFgGHIjmMprRSTuUVWwXyYzZ");
+ }
+ if (modifier == 'E') {
+ return contains("cCxXyYz");
+ }
+ if (modifier == 'O') {
+ return contains("deHImMSuUVwWyz");
+ }
+ return false;
+}
+
+inline void AppendChronoField(std::string* out, FormatArgument argument, char
specifier,
+ char modifier = '\0') {
+ *out += {'{', static_cast<char>(argument), ':', 'L', '%'};
+ if (modifier != '\0') out->push_back(modifier);
+ *out += {specifier, '}'};
+}
+
+inline void AppendLocalizedField(std::string* out, FormatArgument argument) {
+ *out += {'{', static_cast<char>(argument), ':', 'L', '}'};
+}
+
+inline std::string ToChronoFormat(const char* fmt, bool
use_microseconds_suffix) {
Review Comment:
It would be nice to take a `std::string_view` instead of `const char*` IMHO.
##########
cpp/src/arrow/compute/kernels/temporal_internal.h:
##########
@@ -163,15 +174,174 @@ struct ZonedLocalizer {
local_days ConvertDays(sys_days d) const { return
local_days(year_month_day(d)); }
};
+#if ARROW_USE_STD_CHRONO
+namespace detail {
+
+// Argument positions passed to std::vformat_to by TimestampFormatter below.
+enum class FormatArgument : char {
+ ZonedTime = '0',
+ TimeOfDay = '1',
+ TimeOfDayCount = '2',
+};
+
+inline void AppendEscapedLiteral(std::string* out, char value) {
+ out->push_back(value);
+ if (value == '{' || value == '}') {
+ out->push_back(value);
+ }
+}
+
+// These are the directives accepted by Arrow's existing strftime syntax. Treat
+// all others as literals to preserve compatibility.
+inline bool IsSupportedStrftimeSpecifier(char modifier, char specifier) {
+ const auto contains = [specifier](std::string_view candidates) {
+ return candidates.find(specifier) != std::string_view::npos;
+ };
+ if (modifier == '\0') {
+ return contains("aAbBhcCxdeDFgGHIjmMprRSTuUVWwXyYzZ");
+ }
+ if (modifier == 'E') {
+ return contains("cCxXyYz");
+ }
+ if (modifier == 'O') {
+ return contains("deHImMSuUVwWyz");
+ }
+ return false;
+}
+
+inline void AppendChronoField(std::string* out, FormatArgument argument, char
specifier,
+ char modifier = '\0') {
+ *out += {'{', static_cast<char>(argument), ':', 'L', '%'};
+ if (modifier != '\0') out->push_back(modifier);
+ *out += {specifier, '}'};
+}
+
+inline void AppendLocalizedField(std::string* out, FormatArgument argument) {
+ *out += {'{', static_cast<char>(argument), ':', 'L', '}'};
+}
+
+inline std::string ToChronoFormat(const char* fmt, bool
use_microseconds_suffix) {
+ std::string out;
+ bool zoned_field_open = false;
+ const auto close_zoned_field = [&] {
+ if (zoned_field_open) {
+ out.push_back('}');
+ zoned_field_open = false;
+ }
+ };
+ const auto append_literal = [&](char value) {
+ // Braces and literal percent signs must stay outside chrono replacement
fields.
+ if (value == '{' || value == '}' || value == '%') close_zoned_field();
+ AppendEscapedLiteral(&out, value);
Review Comment:
If this is the single call site of `AppendEscapedLiteral`, perhaps it
needn't be a separate function at all?
##########
cpp/src/arrow/compute/kernels/scalar_temporal_test.cc:
##########
@@ -2003,6 +2006,120 @@ TEST_F(ScalarTemporalTest,
TestAssumeTimezoneNonexistent) {
&options_earliest);
}
+#if ARROW_USE_STD_CHRONO
Review Comment:
Some of these tests are not compute-specific, can we move them to a more
appropriate place? For example `arrow/util/time_test.cc`
##########
cpp/src/arrow/compute/kernels/temporal_internal.h:
##########
@@ -182,7 +352,29 @@ struct TimestampFormatter {
const auto timepoint = sys_time<Duration>(Duration{arg});
auto format_zoned_time = [&](auto&& zt) {
try {
- chrono::to_stream(bufstream, format.c_str(), zt);
+#if ARROW_USE_STD_CHRONO
+ // %Q/%q refer to local time of day rather than elapsed time since the
epoch.
+ const auto local_time = zt.get_local_time();
+ // Truncate toward zero, as in StringFormatter<TimestampType>, so
midnight
+ // remains representable even near the lower bound of nanosecond
timestamps.
+ const auto local_day =
+ std::chrono::time_point_cast<std::chrono::days>(local_time);
+ const auto time_of_day = local_day <= local_time
+ ? local_time - local_day
+ : std::chrono::days{1} - (local_day -
local_time);
+ const auto time_of_day_count = time_of_day.count();
+ const std::ostream::sentry sentry(bufstream);
+ if (sentry) {
+ const auto end = std::vformat_to(
+ std::ostreambuf_iterator<char>(bufstream), bufstream.getloc(),
format,
+ std::make_format_args(zt, time_of_day, time_of_day_count));
+ if (end.failed()) bufstream.setstate(std::ios::badbit);
+ } else {
+ bufstream.setstate(std::ios::badbit);
Review Comment:
Why???
--
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]
