kou commented on PR #54661:
URL: https://github.com/apache/arrow/pull/54661#issuecomment-6434651337
Thanks for analyzing. I also analyzed the `date_parsers` approach with
Claude Code. It seems that other systems such as DuckDB and Apache Spark have a
separated date format option. (Time format option isn't reused.)
@pitrou @jorisvandenbossche Do you have any opinion how to process custom
date format columns in CSV reader?
The following text is generated by Claude Code after my analysis. (I changed
some generated parts manually.)
---
### Proposal
* Add `ConvertOptions::date_parsers`. Its type is the same as
`timestamp_parsers`: `std::vector<std::shared_ptr<TimestampParser>>`. We don't
need a new parser class. pyarrow accepts format strings and `ISO8601` like
`timestamp_parsers`.
* `date_parsers` replaces the built-in ISO-8601 parser like
`timestamp_parsers` does:
* If `date_parsers` is empty, we use only the built-in ISO-8601 parser
(`YYYY-MM-DD`) as before. No behavior change.
* If `date_parsers` isn't empty, we use only `date_parsers`. Users can
include `ISO8601()` to accept ISO-8601 values too, e.g. `[ISO8601(),
"%m/%d/%Y"]`. `ISO8601()` also accepts date-only values such as `2000-01-01`.
* A parsed timestamp is floored to the day boundary. The time part is
truncated only when users put a parser that accepts a time part into
`date_parsers`, e.g. `"%m/%d/%Y %H:%M"` or `ISO8601()`. With `"%m/%d/%Y"`,
`03/15/2020 14:30` is rejected because of the trailing characters.
* Values with a zone offset are rejected as the current implementation does.
* `date_parsers` is also used for type inference (the date stage) like
`timestamp_parsers` is used for the timestamp stages.
* `timestamp_parsers` isn't changed.
* We can use the same approach for time columns (#54662) with `time_parsers`.
### Rationale
1. Consistency with other systems: DuckDB (`dateformat`), Spark
(`dateFormat`) and readr (`locale(date_format)` / `col_date(format)`) have a
separate format option for dates and use it for type inference too. With a
custom format, they truncate the time part only when the format has a time
part. I couldn't find any system that applies timestamp formats to date
columns. (DuckDB and Spark also truncate ISO-8601 date-time values for date
columns by default. This proposal keeps the current stricter behavior for the
built-in ISO-8601 parser.)
2. No change to an existing API: With the current approach,
`timestamp_parsers` starts affecting date (and later time) columns, and values
that were rejected are now accepted with the time part truncated. This can't be
reverted after a release. A separate option is purely additive.
3. Consistency with the CSV reader: The CSV reader rejects lossy conversions
for typed columns. For example, `2000-01-01 00:00:00.5` for `timestamp[s]`,
`12:00:00.5` for `time32[s]`, `1.25` for `decimal128(5, 1)` and `300` for
`int8` are all rejected. Truncating the time part silently because of parsers
specified for timestamp columns isn't consistent with this. Truncation
explicitly requested via `date_parsers` is fine.
4. It covers the reported use cases: GH-26224, GH-28303 and GH-33357
(date-only custom formats) work by putting the formats into `date_parsers`.
GH-37180 (`2000-01-01 00:00:00` for a date column) works with `ISO8601()` or
`"%Y-%m-%d %H:%M:%S"` in `date_parsers`.
5. Simpler implementation:
* `StrptimeFormatCanProduce` isn't needed. The problem it solves is that
parsers for timestamp columns are also applied to date columns. It doesn't
happen with `date_parsers`. If users put a wrong format such as `"%H:%M:%S"`
into `date_parsers`, they get what they specified.
* `is_type_inference` isn't needed because type inference uses the same
converter. We don't need to change the public `Converter::Make()`.
* `SplitISO8601Subseconds` isn't needed. `ISO8601()` rejects values with
fractional seconds for a date column like it does for a `timestamp[s]` column.
* We don't need to check `TimestampParser::kind()`. Custom
`TimestampParser` implementations work in the same way.
* The date converter can use the same structure as
`MakeTimestampConverter()`: the built-in parser if no parsers are given, the
given parsers otherwise.
### Changes from the current PR
* Use `date_parsers` instead of `timestamp_parsers` for date columns, and
replace the built-in parser instead of falling back from it.
* Add `date_parsers` to C++ `ConvertOptions`, pyarrow (constructor,
property, equality, pickling, repr) and the docs. (We don't need to change R
(`CsvConvertOptions$create()`) and GLib (`GArrowCSVReadOptions`) in this PR. We
can work on it as separated tasks). pyarrow.dataset doesn't need any change
because it uses `ConvertOptions` as-is.
* Add tests for type inference with `date_parsers`, and remove
`StrptimeFormatCanProduce`, `SplitISO8601Subseconds`, `is_type_inference` and
their tests.
* The flooring, the date32/date64 range checks, the zone offset rejection
and most of the tests (custom formats, pre-epoch values, out of range values,
leap seconds, impossible dates and so on) can be reused.
--
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]
