ianmcook commented on code in PR #50146:
URL: https://github.com/apache/arrow/pull/50146#discussion_r4252011331
##########
cpp/src/arrow/csv/converter.cc:
##########
@@ -464,18 +474,156 @@ struct MultipleParsersTimestampValueDecoder : public
ValueDecoder {
}
protected:
- using ParserVector = std::vector<const TimestampParser*>;
+ TimeUnit::type unit_;
+ bool expect_timezone_;
+ std::vector<const TimestampParser*> parsers_;
+};
+
+//
+// Value decoder for dates and times, with fallback to user-defined
+// timestamp parsers
+//
+
+// Splits the fractional seconds (up to 9 digits) off an ISO-8601 timestamp
+// "YYYY-MM-DD[ T]hh:mm:ss.s{1,9}" with an optional zone offset. Returns `s`
+// if there are no fractional seconds, a copy without them in `buffer`, or
+// nullptr if they are malformed. The split-off digits are returned in
+// `subseconds`.
+const char* SplitISO8601Subseconds(const char* s, uint32_t* size, char
(&buffer)[64],
+ std::string_view* subseconds) {
+ if (*size <= 20 || s[19] != '.') {
+ return s;
+ }
+ uint32_t fraction_length = 0;
+ while (20 + fraction_length < *size && s[20 + fraction_length] >= '0' &&
+ s[20 + fraction_length] <= '9') {
+ ++fraction_length;
+ }
+ const uint32_t rest_length = *size - 20 - fraction_length;
+ if (fraction_length == 0 || fraction_length > 9 || 19 + rest_length >
sizeof(buffer)) {
+ return nullptr;
+ }
+ std::memcpy(buffer, s, 19);
+ std::memcpy(buffer + 19, s + 20 + fraction_length, rest_length);
+ *subseconds = std::string_view(s + 20, fraction_length);
+ *size = 19 + rest_length;
+ return buffer;
+}
+
+// Tries the ISO-8601 format first, then the user-defined timestamp parsers.
+// A timestamp produced by a user-defined parser is floored to the day
+// boundary for dates, and reduced to the time of day for times (consistent
+// with casting a timestamp to date32/date64/time32/time64).
+template <typename T>
+struct DateTimeWithParsersValueDecoder : public ValueDecoder {
+ using value_type = typename T::c_type;
+
+ DateTimeWithParsersValueDecoder(const std::shared_ptr<DataType>& type,
+ const ConvertOptions& options,
+ const TrieCache* trie_cache)
+ : ValueDecoder(type, options, trie_cache),
+ concrete_type_(checked_cast<const T&>(*type)),
+ parse_unit_(GetParseUnit(concrete_type_)),
+ ticks_per_day_(TicksPerDay(parse_unit_)),
+ parsers_(GetTimestampParsers(options_)) {}
+
+ Status Decode(const uint8_t* data, uint32_t size, bool quoted, value_type*
out) {
+ TrimWhiteSpace(&data, &size);
+ if (ARROW_PREDICT_TRUE(string_converter_.Convert(
+ concrete_type_, reinterpret_cast<const char*>(data), size, out))) {
+ return Status::OK();
+ }
+ for (const auto& parser : parsers_) {
+ int64_t timestamp = 0;
+ bool zone_offset_present = false;
+ const char* s = reinterpret_cast<const char*>(data);
+ uint32_t s_size = size;
+ char buffer[64];
+ std::string_view subseconds;
+ if (std::strcmp(parser->kind(), "iso8601") == 0) {
+ // Set the fractional seconds aside, so that any number of digits is
+ // accepted: dates discard them, times add them back below
+ s = SplitISO8601Subseconds(s, &s_size, buffer, &subseconds);
+ if (s == nullptr) {
+ continue;
+ }
+ }
+ TimeUnit::type unit = parse_unit_;
+ bool parsed = parser->operator()(s, s_size, unit, ×tamp,
&zone_offset_present);
Review Comment:
Values with a seconds field of `60` are silently wrapped into the next
minute when a strptime parser is used. With `timestamp_parsers=["%H:%M:%S"]`
and a `time32[s]` column:
- `23:59:60` produces `00:00:00`.
- `12:30:60` produces `12:31:00`.
With `timestamp_parsers=["%Y-%m-%d %H:%M:%S"]` and a `date32` column,
`2020-12-31 23:59:60` produces `2021-01-01`.
Without parsers, `23:59:60` is rejected for time columns, and existing tests
check for this. So setting a parser turns an error into a wrong value. strptime
accepts 60 as a seconds value to allow for leap seconds, and
`ParseTimestampStrptime` then normalizes it into the next minute.
The decoder only sees the already-normalized timestamp, so I think the check
has to go in `ParseTimestampStrptime`: reject `tm_sec >= 60`. The ISO-8601
parser already rejects seconds ≥ 60, so this would make the two parsers
consistent. It would also change timestamp columns, which currently accept
these values (`2020-12-31 23:59:60` becomes `2021-01-01 00:00:00`). I think
that's the right behavior, but it should be called out in the PR description.
Could we add that check, along with regression tests rejecting the examples
above?
I checked every input and output in the comment against the sanitizer build.
I left out glibc because I only tested on macOS.
--
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]
