ianmcook commented on code in PR #50146:
URL: https://github.com/apache/arrow/pull/50146#discussion_r4201259412
##########
cpp/src/arrow/csv/converter.cc:
##########
@@ -464,18 +472,93 @@ struct MultipleParsersTimestampValueDecoder : public
ValueDecoder {
}
protected:
- using ParserVector = std::vector<const TimestampParser*>;
+ TimeUnit::type unit_;
+ bool expect_timezone_;
+ std::vector<const TimestampParser*> parsers_;
+};
- static ParserVector GetParsers(const ConvertOptions& options) {
- ParserVector parsers(options.timestamp_parsers.size());
- for (size_t i = 0; i < options.timestamp_parsers.size(); ++i) {
- parsers[i] = options.timestamp_parsers[i].get();
+//
+// Value decoder for dates and times, with fallback to user-defined
+// timestamp parsers
+//
+
+// 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;
+ if (parser->operator()(reinterpret_cast<const char*>(data), size,
parse_unit_,
+ ×tamp, &zone_offset_present) &&
+ !zone_offset_present) {
+ // Floor division, to handle values before the epoch
+ int64_t days = timestamp / ticks_per_day_;
+ days -= (timestamp % ticks_per_day_) < 0;
+ if constexpr (std::is_same_v<T, Date32Type>) {
+ *out = static_cast<value_type>(days);
+ } else if constexpr (std::is_same_v<T, Date64Type>) {
+ *out = days * kMillisPerDay;
+ } else {
+ static_assert(is_time_type<T>::value);
+ *out = static_cast<value_type>(timestamp - days * ticks_per_day_);
+ }
+ return Status::OK();
+ }
}
- return parsers;
+ return GenericConversionError(type_, data, size);
}
- TimeUnit::type unit_;
- bool expect_timezone_;
+ protected:
+ static constexpr int64_t kMillisPerDay = 86400000;
+
+ static TimeUnit::type GetParseUnit(const T& type) {
+ if constexpr (is_time_type<T>::value) {
+ // Parse in the time type's own unit, so that the time of day can be
+ // extracted without further conversion
+ return type.unit();
+ } else {
+ return TimeUnit::SECOND;
Review Comment:
Using `TimeUnit::SECOND` here causes the ISO fallback to reject
fractional-second timestamps before the date flooring runs. For example:
```python
import pyarrow as pa
import pyarrow.csv as csv
csv.read_csv(
pa.BufferReader(b"a\n2020-03-15 14:30:00.123\n"),
convert_options=csv.ConvertOptions(
column_types={"a": pa.date32()},
timestamp_parsers=[csv.ISO8601],
),
)
```
This fails with a conversion error, whereas removing `.123` succeeds. The
same issue affects `date64`. Under the documented flooring semantics, both
inputs should produce `2020-03-15`.
Could we accept the fractional component during parsing and discard it when
computing the date? Regression tests for both date types, including a pre-epoch
value, would help cover this. The fix should also preserve the supported date
range; unconditionally switching to nanosecond parsing would narrow it.
--
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]
