pearu commented on code in PR #50146:
URL: https://github.com/apache/arrow/pull/50146#discussion_r4221007978
##########
cpp/src/arrow/csv/inference_internal.h:
##########
@@ -46,7 +46,15 @@ enum class InferKind {
class InferStatus {
public:
explicit InferStatus(const ConvertOptions& options)
- : kind_(InferKind::Null), can_loosen_type_(true), options_(options) {}
+ : kind_(InferKind::Null), can_loosen_type_(true), options_(options) {
+ if (!options.timestamp_parsers.empty()) {
+ // Date and time inference must not use the user-defined timestamp
parsers,
+ // otherwise a value with a time-of-day (resp. date) part could be
inferred
+ // as a date (resp. time) and be silently truncated.
+ date_time_options_ = std::make_unique<ConvertOptions>(options);
Review Comment:
Thanks, good catch! Applied your suggestion: `Converter::Make` now takes an
`is_type_inference` flag, and the per-column copy of `ConvertOptions` is gone.
Peak RSS when reading a one-row CSV with all columns listed in
`include_columns` (release build):
| columns | no parsers | `[ISO8601]` before | `[ISO8601]` after |
|---|---|---|---|
| 1000 | 67.5 MB | 99.9 MB | 68.2 MB |
| 2000 | 76.4 MB | 200.5 MB | 76.2 MB |
| 4000 | 95.1 MB | 588.0 MB | 95.2 MB |
| 8000 | 133.0 MB | 2110.6 MB | 133.7 MB |
##########
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:
Thanks, fixed in 4a0fb79c28. Accepting the fraction inside the parser would
mean either parsing in a finer unit, which narrows the date range as you note,
or changing `ParseTimestampISO8601`/`TimestampParser`. Instead the fractional
digits are split off the string before the ISO-8601 parser runs in seconds, so
dates keep the full range and 1–9 fractional digits are accepted, with the rest
of the value still validated by the parser. Added tests for date32 and date64,
including pre-epoch and year-1600 values, all digit counts, and zone offsets
after a fraction still being rejected.
##########
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_);
Review Comment:
Thanks, applied as suggested: the normalized remainder in 52395269c0, with
`1677-09-21 00:12:44` → `time64[ns]` as a regression test, and the date32
bounds check plus checked multiplication for date64 in 90ca767501. Both now
return a conversion error. Since the built-in parsers only read 4-digit years,
the range tests use a small test-only parser that reads integer epoch seconds,
like your reproduction.
##########
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();
Review Comment:
Thanks, fixed in 4e14bcb1a2. The value is still parsed in the time unit
first; when that fails, it is parsed again in seconds and only the time of day
is scaled to the unit. For the ISO-8601 parser the fractional digits are split
off and parsed separately in the time unit, so precision is kept for any date
(`9999-12-31 07:55:00.123456789` → `time64[ns]` works). Tests cover years 1600
and 9999 with both ISO-8601 and `%Y-%m-%d %H:%M:%S`.
--
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]
