pearu commented on code in PR #50146:
URL: https://github.com/apache/arrow/pull/50146#discussion_r4258238535
##########
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);
Review Comment:
Thanks, applied your simpler version in 445f974808: the fractional digits
must now end the value, so the split only shortens `size`, and the buffer and
both `memcpy` calls are gone. Added regression tests rejecting `2020-03-15
14:30:00.5.5` and `23:59:59.9.9` for `time32[ms]`, the double fraction for
`time64[us]`/`time64[ns]`, and the zone offsets after a fraction for a time
column as well.
##########
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:
Thanks, done in 0ff0951a2a: `ParseTimestampStrptime` now rejects `tm_sec >=
60`, with regression tests for your three examples and, at the parser level,
for `:60` and `:61` (glibc accepts 61 too). The timestamp-column change is
called out in the PR description, together with how the standards and strptime
implementations differ on these values.
While probing similar inputs, the same function also rolled impossible dates
over into the next month (`30-FEB-15` → `2015-03-02`, in date and timestamp
columns, since strptime only checks the day to be in 1..31), so 1e9bf12849
rejects those as well, again consistent with the ISO-8601 parser.
##########
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_)) {}
Review Comment:
Thanks, done in 20deafe068 along the lines you suggested: the decoder now
skips strptime parsers whose format cannot produce the column's kind of value
(no date directive for date columns, no time directive for time columns; AM/PM
alone doesn't count, as it only qualifies an hour), and date-only ISO-8601
values are rejected in time columns. Formats with both (`%Y-%m-%d %H:%M:%S`)
and parsers of other kinds are still tried. Your four examples are now
regression tests, and the C++/Python docs and the user guide describe the rule.
An alternative raised in #41488 and on #33357 is separate
`date_parsers`/`time_parsers` options; that would be an additive follow-up if
wanted, and the format classification is a standalone helper so it could be
reused or dropped there.
--
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]
