pearu commented on PR #54661: URL: https://github.com/apache/arrow/pull/54661#issuecomment-6416859532
I used an AI agent to analyze the suggested alternative for its pros and cons. Assuming the existing tests stay as representative samples of the feature, with a separate `date_parsers`: Could go: - `StrptimeFormatCanProduce` - `make_date_time_converter` and the `is_type_inference` parameter of `Converter::Make`, but only if `date_parsers` is also used for type inference, which means a column matching a datetime format listed in both options would be inferred as a date (inference tries dates before timestamps); keeping them means `date_parsers` differs from `timestamp_parsers` in not affecting inference Would come: - the new option in C++ `ConvertOptions` and in pyarrow (constructor, property, equality, pickling, repr), with its tests and docs, likely later also in GLib and R - if used for type inference: inference of non-ISO date columns as dates, with its own tests - the same again with `time_parsers` for #54662 Everything else would stay, as the tests use datetime formats, `ISO8601()` with fractional seconds and custom parsers, which the new option would have to accept as `timestamp_parsers` does; only the tests that a format without a date directive is not tried (and their counterparts in #54662) would be dropped, as that case only arises when date and timestamp columns share one option. Users with formats that contain both a date and a time would list them in both options. The API change is the more important point: a new public option that is documented and kept from then on. So the implementation would get somewhat simpler, at the cost of a larger API. On this balance I'd keep the current approach, unless you see an API reason for the separate option. -- 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]
