ianmcook commented on code in PR #50146:
URL: https://github.com/apache/arrow/pull/50146#discussion_r4252069030
##########
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:
Because `timestamp_parsers` applies to every date and time column, a value
in the wrong column can be silently accepted. A CSV with both kinds of column
from the PR description (a date like `15-OCT-15` and a time like `7:55:00`)
needs both `"%d-%b-%y"` and `"%H:%M:%S"`, and then:
- `7:55:00` in a `date32` column produces `1900-01-01`.
- `15-OCT-15` in a `time32[s]` column produces `00:00:00`.
The same happens with `timestamp_parsers=[ISO8601]` alone: `2020-03-15` in a
`time32[s]` column produces `00:00:00`.
This matches casting a timestamp to a date or time. But it means a shifted
column or a data-entry error turns into plausible-looking data instead of a
conversion error.
Could we skip parsers that can't produce the column's type? For example, the
`DateTimeWithParsersValueDecoder` constructor could keep only those strptime
parsers whose format has a date directive (`%d`, `%m`, `%y`, `%Y`, `%b`, `%j`,
...) for date columns, and only those with a time directive (`%H`, `%I`, `%M`,
`%S`, `%T`, `%R`, ...) for time columns. For time columns, the ISO-8601
fallback could also reject date-only values.
If you'd rather keep the current behavior, could the docs at least say that
the parsers apply to all date and time columns, so that a value matching any of
them is accepted with the missing part defaulted (1900-01-01 for dates,
midnight for times)?
I ran all four examples against the sanitizer build with both parsers set
together, and they produce exactly the results quoted above.
--
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]
