Repository navigation
GH-41488: [C++][Python] Apply timestamp_parsers as fallback when parsing CSV date and time columns - #50146
GH-41488: [C++][Python] Apply timestamp_parsers as fallback when parsing CSV date and time columns#50146pearu wants to merge 7 commits into
Conversation
|
Two out-of-scope discoveries made while working on this, recorded here rather than folded into the PR to keep it minimal:
|
|
|
|
|
78ca3cb to
e0af29e
Compare
|
A third discovery for the list above: this |
e0af29e to
e75ecec
Compare
|
|
|
The single CI failure ( |
There was a problem hiding this comment.
Pull request overview
This PR extends the CSV reader’s ConvertOptions::timestamp_parsers behavior so that, when columns are explicitly typed as date32/date64/time32/time64, the reader first attempts the existing ISO-8601 parsing and then falls back to the user-provided timestamp parsers (with flooring/extracting semantics consistent with casting). It also keeps type inference strict (ISO-only) to avoid silent truncation.
Changes:
- Add a new C++ CSV date/time value decoder that falls back to
timestamp_parsersafter ISO parsing and applies flooring/time-of-day extraction. - Ensure CSV type inference for date/time remains ISO-only even when
timestamp_parsersare configured. - Update C++/Python docs and add C++/Python tests; improve vendored Windows
strptimesupport for C-locale day/month names.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| python/pyarrow/tests/test_csv.py | Adds Python coverage for date/time typed columns using timestamp_parsers fallback and inference guard. |
| python/pyarrow/_csv.pyx | Documents the new fallback behavior in Python ConvertOptions docstring. |
| docs/source/cpp/csv.rst | Adds a “Date and time parsing” section documenting fallback + semantics. |
| cpp/src/arrow/vendored/musl/strptime.c | Adds a C-locale nl_langinfo fallback table for Windows/testing to support %b/%B/%p/.... |
| cpp/src/arrow/csv/options.h | Documents fallback semantics for timestamp_parsers in C++ API docs. |
| cpp/src/arrow/csv/inference_internal.h | Ensures date/time inference ignores configured timestamp_parsers. |
| cpp/src/arrow/csv/converter.cc | Implements fallback decoder + converter factory changes for date/time types. |
| cpp/src/arrow/csv/converter_test.cc | Adds C++ tests for date/time fallback parsing behavior and edge cases. |
e75ecec to
fcf4f95
Compare
|
@jorisvandenbossche Do you want to take a look at this PR? |
|
Hi, a small quick question — I noticed that you mentioned multiple issues in the PR description. I was wondering, does this PR fix all of them? Thanks! |
|
@manyifire Good question — mostly, but not every one in the same way:
So: yes for #28303/#33357 (and #26783/#26224), yes-on-Windows for #31816/#31971, and for #37180 only when |
fcf4f95 to
808f110
Compare
|
Rebased onto current Status recap for whoever picks this up:
@pitrou — you routed this to @jorisvandenbossche back in June and it has been quiet since. Is there anything I can do to make this easier to review? If reviewing the CSV converter change together with the vendored 🤖 Drafted by Claude Code (an AI agent) and reviewed & approved by pearu. |
d866653 to
853447b
Compare
…n parsing CSV date and time columns CSV columns explicitly typed as date32, date64, time32 or time64 could only be parsed from strict ISO-8601 strings; ConvertOptions::timestamp_parsers was consulted only for timestamp columns. Make the user-defined timestamp parsers act as a fallback for these column types: the built-in ISO-8601 parser is tried first (preserving existing behavior), then each configured parser in order. A timestamp produced by a fallback parser is floored to the day boundary for dates and reduced to the time of day for times, consistent with casting a timestamp to a date or time type. Type inference of date and time columns is deliberately unaffected: inference keeps using strict ISO-8601 parsing, otherwise a value with a time-of-day part could be inferred as a date and silently truncated. Also provide C-locale name tables to the vendored musl strptime used on Windows, where nl_langinfo() is unavailable: this makes %a/%A/%b/%B/%h/ %p/%c/%r/%x/%X work on Windows (matching musl's C locale), so that the month-name formats from the original issue reports parse on all platforms. Closes apacheGH-28303. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
853447b to
09ac291
Compare
| // 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); |
There was a problem hiding this 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:
return Converter::Make(type, options_, pool,
/*is_type_inference=*/true);Then MakeDateTimeConverter could select the existing decoder directly:
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.
There was a problem hiding this 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 |
| // extracted without further conversion | ||
| return type.unit(); | ||
| } else { | ||
| return TimeUnit::SECOND; |
There was a problem hiding this comment.
Using TimeUnit::SECOND here causes the ISO fallback to reject fractional-second timestamps before the date flooring runs. For example:
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.
There was a problem hiding this comment.
Thanks, fixed in 4a0fb79. 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.
| *out = days * kMillisPerDay; | ||
| } else { | ||
| static_assert(is_time_type<T>::value); | ||
| *out = static_cast<value_type>(timestamp - days * ticks_per_day_); |
There was a problem hiding this comment.
The multiplication days * ticks_per_day_ can overflow even when the parsed timestamp and resulting time of day are both representable.
For example, parsing 1677-09-21 00:12:44 into time64[ns] with timestamp_parsers=[ISO8601] succeeds at the timestamp-parsing step, but UBSan then reports:
signed integer overflow: -106752 * 86400000000000 cannot be represented in type 'int64_t'
Could we compute the time of day using a normalized remainder instead?
int64_t time_of_day = timestamp % ticks_per_day_;
if (time_of_day < 0) {
time_of_day += ticks_per_day_;
}
*out = static_cast<value_type>(time_of_day);This preserves the intended behavior for pre-epoch timestamps without the overflowing intermediate multiplication. A regression test using the input above would cover this boundary.
There was a problem hiding this comment.
The date branches in this block need bounds checks too. With a custom C++ TimestampParser that reads integer epoch seconds, I reproduced two additional cases:
- For
date32,185542587187200seconds corresponds to2147483648days. The unchecked cast silently wraps this to-2147483648. - For
date64,INT64_MAXseconds triggers a UBSan signed-overflow error indays * kMillisPerDay.
Both should return a conversion error because the result is outside the target type’s range.
Could we also check days against the int32_t bounds before narrowing to date32, and use checked multiplication for date64? The normalized-remainder change suggested above addresses the time branch, but these date branches need separate checks.
There was a problem hiding this comment.
Thanks, applied as suggested: the normalized remainder in 5239526, with 1677-09-21 00:12:44 → time64[ns] as a regression test, and the date32 bounds check plus checked multiplication for date64 in 90ca767. 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.
|
@pearu Thanks for your patience and persistence. I reviewed this and added some comments with assistance from GPT-6 Astra. |
| 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(); |
There was a problem hiding this comment.
Using the time type’s unit for the intermediate timestamp unnecessarily restricts time64[ns] to the timestamp[ns] date range.
For example, with input 9999-12-31 07:55:00:
- Reading directly as
time64[ns]fails with a conversion error. - Reading as
timestamp[s]and casting totime64[ns]succeeds, producing07:55:00.000000000.
I reproduced this with both ISO8601 and %Y-%m-%d %H:%M:%S, and with year 1600 as well. The resulting time of day is representable; the failure comes from scaling the entire timestamp to nanoseconds before discarding the date.
Could we extract the time at a usable intermediate precision before scaling to the output unit, while preserving fractional precision? Regression tests with dates outside the nanosecond timestamp range would cover this.
This failure occurs during parsing, so replacing the later multiplication with a normalized remainder won’t resolve it.
There was a problem hiding this comment.
Thanks, fixed in 4e14bcb. 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.
Pass an is_type_inference flag through Converter::Make to MakeDateTimeConverter instead of keeping a per-column copy of ConvertOptions with timestamp_parsers cleared. The copy made memory usage quadratic in the number of columns when timestamp_parsers was set. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The ISO-8601 fallback parsed dates in seconds, so values with fractional seconds were rejected before the flooring to the day could run. Discard the fractional digits before parsing, which keeps the full date range of parsing in seconds. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
days * ticks_per_day can overflow near the minimum of nanosecond timestamps even though the time of day is representable. Use a normalized remainder instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Parsing in the time unit limits time64[ns] columns to dates between 1677 and 2262 although the time of day is representable. Retry in seconds when parsing in the unit fails, and for the ISO-8601 parser split the fractional seconds off so that they are parsed separately in the time unit, keeping their precision for any date. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Narrowing the day count to date32 wrapped silently, and scaling it to milliseconds for date64 could overflow. Both are reachable with a user-defined timestamp parser, so return a conversion error instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Rationale for this change
CSV columns explicitly typed as
date32,date64,time32ortime64can only be parsed from strict ISO-8601 strings:ConvertOptions::timestamp_parsersis consulted only fortimestampcolumns. Reading e.g.15-OCT-15into adate32column fails even withtimestamp_parsers=["%d-%b-%y"], and7:55:00(non-zero-padded hour) fails fortime32[s]. Users currently work around this by declaring such columns astimestamp, reading, then casting back to the date/time type.Effect on the issues collected in #41488:
%d-%b-%y).date32column withtimestamp_parsers=["%Y/%m/%d"]— now works as requested).ArrowInvalid: CSV conversion error to date32[day]: invalid value '2000-01-01 00:00:00' #37180 is addressed but not auto-closed: it asks for ISO timestamp strings (2000-01-01 00:00:00) in adate32column to convert by default. With this PR that works by opting in viatimestamp_parsers=[ISO8601]; the no-parsers default still errors, deliberately, so that time-of-day truncation only happens when the user asked for it. Whether the remaining default-behavior ask should be implemented or declined is left to that issue.%b/%Bfail on Windows) and [C++][Python] strptime fails to parse with %p on Windows #31971 (%pfails on Windows) are also resolved for alltimestamp_parsersusers;%zremains unsupported on Windows (kStrptimeSupportsZone, unchanged).What changes are included in this PR?
DateTimeWithParsersValueDecoderincsv/converter.cc, used for date32/date64/time32/time64 columns whentimestamp_parsersis non-empty. It tries the built-in ISO-8601 parser first (preserving all existing behavior), then each configured parser in order. A timestamp produced by a fallback parser is floored to the day boundary for dates and reduced to the time of day for times, consistent with casting a timestamp to a date or time type. Values carrying a zone offset are rejected, as for zone-less timestamp columns. When no parsers are configured, the pre-existing decoder is used unchanged.timestamp_parserscleared, so inference keeps strict ISO-8601 semantics (otherwise a value with a time-of-day part could be inferred as a date and silently truncated). The existingtest_timestamp_parsersPython test pins this behavior.ConvertOptions::timestamp_parsers(C++ and Python docstrings) and a new "Date and time parsing" section in the C++ CSV user guide.strptimeused on Windows, wherenl_langinfo()is unavailable. Previously the%a/%A/%b/%B/%h/%p/%c/%r/%x/%Xspecifiers were compiled out on Windows, so the month-name formats from the original issue reports (%d-%b-%y) could not work there for any column type. The tables match musl's C locale, and name matching is case-insensitive as on glibc/musl/BSD. The fallback path is compiled and verified on Linux via theARROW_TEST_FALLBACK_LANGINFOhook.Are these changes tested?
Yes:
Date32Conversion.UserDefinedParsers,Date64Conversion.UserDefinedParsers,Time32Conversion.UserDefinedParsers,Time64Conversion.UserDefinedParsers) covering custom formats, mixed ISO + custom values in one column (backward compatibility of ISO values when parsers are set), pre-epoch flooring with a time-of-day component (distinguishes floor from truncating division), time-of-day extraction from pre-epoch timestamps, zone-offset rejection, and error cases.Are there any user-facing changes?
Yes:
ConvertOptions::timestamp_parsersnow also applies, as a fallback after ISO-8601, to columns explicitly typed as date32/date64/time32/time64 (previously such values always errored). No breaking changes: behavior withouttimestamp_parsersis untouched, ISO values keep parsing when parsers are set, and type inference is unchanged. All language bindings gain the behavior without API changes.AI usage disclosure
This PR was developed with AI assistance (Claude Code): the decoder, tests and documentation were AI-generated under my direction, then reviewed line-by-line and iterated on by me (design decisions: fallback-after-ISO semantics, silent flooring, inference isolation, and several implementation details adjusted during review). I own and can debug these changes.
🤖 Generated with Claude Code