ianmcook commented on code in PR #50146:
URL: https://github.com/apache/arrow/pull/50146#discussion_r4201237913
##########
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:
Could we avoid copying all of `ConvertOptions` for every inferred column?
Both `include_columns` and `column_types` can grow with the number of columns,
so this introduces quadratic memory usage. In a local UBSan debug build,
reading a one-row CSV with 2,000 projected integer columns used about 69 MB
peak memory without custom parsers and 170 MB with `[ISO8601]`.
One small fix would be to add an `is_type_inference` flag, defaulting to
`false`, to `Converter::Make` and pass it through to `MakeDateTimeConverter`.
The `Date` and `Time` inference cases would call:
```cpp
return Converter::Make(type, options_, pool,
/*is_type_inference=*/true);
```
Then `MakeDateTimeConverter` could select the existing decoder directly:
```cpp
if (is_type_inference || options.timestamp_parsers.empty()) {
return std::make_shared<ConverterType<T, NumericValueDecoder<T>>>(
type, options, pool);
}
```
That would let us remove `date_time_options_` entirely while preserving
strict date/time inference, custom timestamp inference, and the original
options by reference.
--
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]
