Skip to content

GH-41488: [C++][Python] Apply timestamp_parsers as fallback when parsing CSV date and time columns #50146

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pearu wants to merge 7 commits into
base: main
Choose a base branch
from
+524 −21
Open

GH-41488: [C++][Python] Apply timestamp_parsers as fallback when parsing CSV date and time columns #50146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
09ac291
GH-41488: [C++][Python] Apply timestamp_parsers as fallback when pars…
pearu Jun 9, 2026
d19c0b3
Avoid copying ConvertOptions for each inferred column
pearu Oct 7, 2026
4a0fb79
Accept ISO-8601 fractional seconds in date columns
pearu Oct 8, 2026
5239526
Extract the time of day without overflowing
pearu Oct 8, 2026
4e14bcb
Extract the time of day from dates outside the time unit's range
pearu Oct 8, 2026
90ca767
Reject dates outside the range of date32 and date64
pearu Oct 8, 2026
05df5b6
Merge branch 'main' into pearu/fix-csv-date-time-parsers
pearu Oct 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
214 changes: 200 additions & 14 deletions cpp/src/arrow/csv/converter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <limits>
#include <sstream>
#include <string>
#include <string_view>
#include <type_traits>
#include <vector>

Expand All @@ -36,6 +37,7 @@
#include "arrow/type_traits.h"
#include "arrow/util/checked_cast.h"
#include "arrow/util/decimal.h"
#include "arrow/util/int_util_overflow.h"
#include "arrow/util/trie_internal.h"
#include "arrow/util/utf8_internal.h"
#include "arrow/util/value_parsing.h" // IWYU pragma: keep
Expand Down Expand Up @@ -440,6 +442,14 @@ struct SingleParserTimestampValueDecoder : public ValueDecoder {
const TimestampParser& parser_;
};

std::vector<const TimestampParser*> GetTimestampParsers(const ConvertOptions& options) {
std::vector<const TimestampParser*> parsers(options.timestamp_parsers.size());
for (size_t i = 0; i < options.timestamp_parsers.size(); ++i) {
parsers[i] = options.timestamp_parsers[i].get();
}
return parsers;
}

struct MultipleParsersTimestampValueDecoder : public ValueDecoder {
using value_type = int64_t;

Expand All @@ -449,7 +459,7 @@ struct MultipleParsersTimestampValueDecoder : public ValueDecoder {
: ValueDecoder(type, options, trie_cache),
unit_(checked_cast<const TimestampType&>(*type_).unit()),
expect_timezone_(!checked_cast<const TimestampType&>(*type_).timezone().empty()),
parsers_(GetParsers(options_)) {}
parsers_(GetTimestampParsers(options_)) {}

Status Decode(const uint8_t* data, uint32_t size, bool quoted, value_type* out) {
bool zone_offset_present = false;
Expand All @@ -464,18 +474,156 @@ struct MultipleParsersTimestampValueDecoder : public ValueDecoder {
}

protected:
using ParserVector = std::vector<const TimestampParser*>;
TimeUnit::type unit_;
bool expect_timezone_;
std::vector<const TimestampParser*> parsers_;
};

//
// Value decoder for dates and times, with fallback to user-defined
// timestamp parsers
//

// Splits the fractional seconds (up to 9 digits) off an ISO-8601 timestamp
// "YYYY-MM-DD[ T]hh:mm:ss.s{1,9}" with an optional zone offset. Returns `s`
// if there are no fractional seconds, a copy without them in `buffer`, or
// nullptr if they are malformed. The split-off digits are returned in
// `subseconds`.
const char* SplitISO8601Subseconds(const char* s, uint32_t* size, char (&buffer)[64],
std::string_view* subseconds) {
if (*size <= 20 || s[19] != '.') {
return s;
}
uint32_t fraction_length = 0;
while (20 + fraction_length < *size && s[20 + fraction_length] >= '0' &&
s[20 + fraction_length] <= '9') {
++fraction_length;
}
const uint32_t rest_length = *size - 20 - fraction_length;
if (fraction_length == 0 || fraction_length > 9 || 19 + rest_length > sizeof(buffer)) {
return nullptr;
}
std::memcpy(buffer, s, 19);
std::memcpy(buffer + 19, s + 20 + fraction_length, rest_length);

@ianmcook ianmcook Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we validate the suffix after the fractional digits before copying it into buffer? The new splitting logic accepts timestamps with two fractional components.

For an explicit time32[ms] column with timestamp_parsers=[ISO8601]:

  • 2020-03-15 14:30:00.5.5 incorrectly succeeds as 14:30:01.000.
  • 2020-03-15 23:59:59.9.9 produces 86400800 milliseconds, outside the valid daily range, so the resulting table fails ValidateFull().

The splitter removes the first fraction, leaving the second for the ISO parser to consume, and then the decoder adds the first fraction back. This also affects time64[us] and time64[ns].

Please ensure the suffix is empty or a valid zone offset before reconstructing the string, and add regression tests rejecting both examples.

@ianmcook ianmcook Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A possibly simpler way to fix this: date and time columns reject zone offsets anyway, so the only valid thing after the fractional digits is the end of the value. If SplitISO8601Subseconds requires the fraction to run to the end, it can just shorten size to 19 instead of copying the rest into a buffer. That rejects a second fraction outright and also removes the 64-byte stack buffer and both memcpy calls:

// Splits the fractional seconds (1 to 9 digits) off the end of an ISO-8601
// timestamp "YYYY-MM-DD[ T]hh:mm:ss.s{1,9}" into `subseconds`, and shortens
// `size` to exclude them. Returns false if the fractional seconds are
// malformed or followed by anything (zone offsets are rejected for dates and
// times anyway).
bool SplitISO8601Subseconds(const char* s, uint32_t* size,
                            std::string_view* subseconds) {
  if (*size <= 20 || s[19] != '.') {
    return true;
  }
  const uint32_t fraction_length = *size - 20;
  if (fraction_length > 9) {
    return false;
  }
  for (uint32_t i = 20; i < *size; ++i) {
    if (s[i] < '0' || s[i] > '9') {
      return false;
    }
  }
  *subseconds = std::string_view(s + 20, fraction_length);
  *size = 19;
  return true;
}

with the call site becoming:

      if (std::strcmp(parser->kind(), "iso8601") == 0 &&
          !SplitISO8601Subseconds(s, &s_size, &subseconds)) {
        continue;
      }

The existing tests that reject zone offsets after a fraction (.5Z, .5+05, .5+0530, .5+05:30) should still pass, since those values now fail at the split instead of in the parser. The regression tests for 2020-03-15 14:30:00.5.5 and 2020-03-15 23:59:59.9.9 would still be good to add.

*subseconds = std::string_view(s + 20, fraction_length);
*size = 19 + rest_length;
return buffer;
}

// 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_)) {}

@ianmcook ianmcook Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because timestamp_parsers applies to every date and time column, a value in the wrong column can be silently accepted. A CSV with both kinds of column from the PR description (a date like 15-OCT-15 and a time like 7:55:00) needs both "%d-%b-%y" and "%H:%M:%S", and then:

  • 7:55:00 in a date32 column produces 1900-01-01.
  • 15-OCT-15 in a time32[s] column produces 00:00:00.

The same happens with timestamp_parsers=[ISO8601] alone: 2020-03-15 in a time32[s] column produces 00:00:00.

This matches casting a timestamp to a date or time. But it means a shifted column or a data-entry error turns into plausible-looking data instead of a conversion error.

Could we skip parsers that can't produce the column's type? For example, the DateTimeWithParsersValueDecoder constructor could keep only those strptime parsers whose format has a date directive (%d, %m, %y, %Y, %b, %j, ...) for date columns, and only those with a time directive (%H, %I, %M, %S, %T, %R, ...) for time columns. For time columns, the ISO-8601 fallback could also reject date-only values.

If you'd rather keep the current behavior, could the docs at least say that the parsers apply to all date and time columns, so that a value matching any of them is accepted with the missing part defaulted (1900-01-01 for dates, midnight for times)?

I ran all four examples against the sanitizer build with both parsers set together, and they produce exactly the results quoted above.


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;
const char* s = reinterpret_cast<const char*>(data);
uint32_t s_size = size;
char buffer[64];
std::string_view subseconds;
if (std::strcmp(parser->kind(), "iso8601") == 0) {
// Set the fractional seconds aside, so that any number of digits is
// accepted: dates discard them, times add them back below
s = SplitISO8601Subseconds(s, &s_size, buffer, &subseconds);
if (s == nullptr) {
continue;
}
}
TimeUnit::type unit = parse_unit_;
bool parsed = parser->operator()(s, s_size, unit, &timestamp, &zone_offset_present);

@ianmcook ianmcook Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Values with a seconds field of 60 are silently wrapped into the next minute when a strptime parser is used. With timestamp_parsers=["%H:%M:%S"] and a time32[s] column:

  • 23:59:60 produces 00:00:00.
  • 12:30:60 produces 12:31:00.

With timestamp_parsers=["%Y-%m-%d %H:%M:%S"] and a date32 column, 2020-12-31 23:59:60 produces 2021-01-01.

Without parsers, 23:59:60 is rejected for time columns, and existing tests check for this. So setting a parser turns an error into a wrong value. strptime accepts 60 as a seconds value to allow for leap seconds, and ParseTimestampStrptime then normalizes it into the next minute.

The decoder only sees the already-normalized timestamp, so I think the check has to go in ParseTimestampStrptime: reject tm_sec >= 60. The ISO-8601 parser already rejects seconds ≥ 60, so this would make the two parsers consistent. It would also change timestamp columns, which currently accept these values (2020-12-31 23:59:60 becomes 2021-01-01 00:00:00). I think that's the right behavior, but it should be called out in the PR description.

Could we add that check, along with regression tests rejecting the examples above?

I checked every input and output in the comment against the sanitizer build. I left out glibc because I only tested on macOS.

if (!parsed && unit != TimeUnit::SECOND) {
// The date may be out of the range of the time unit while the time of
// day is representable: retry in seconds, scaled below
unit = TimeUnit::SECOND;
parsed = parser->operator()(s, s_size, unit, &timestamp, &zone_offset_present);
}
if (parsed && !zone_offset_present) {
const int64_t ticks_per_day = TicksPerDay(unit);
// 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>) {
if (days < std::numeric_limits<value_type>::min() ||
days > std::numeric_limits<value_type>::max()) {
continue; // out of the range of date32
}
*out = static_cast<value_type>(days);
} else if constexpr (std::is_same_v<T, Date64Type>) {
if (arrow::internal::MultiplyWithOverflow(days, kMillisPerDay, out)) {
continue; // out of the range of date64
}
} else {
static_assert(is_time_type<T>::value);
// Normalized remainder, as days * ticks_per_day can overflow
int64_t time_of_day = timestamp % ticks_per_day;
if (time_of_day < 0) {
time_of_day += ticks_per_day;
}
uint32_t subseconds_ticks = 0;
if (!subseconds.empty() &&
!arrow::internal::detail::ParseSubSeconds(
subseconds.data(), subseconds.size(), parse_unit_, &subseconds_ticks)) {
continue; // the time unit cannot represent the fractional seconds
}
*out = static_cast<value_type>(time_of_day * (ticks_per_day_ / ticks_per_day) +
subseconds_ticks);
}
return Status::OK();
}
}
return GenericConversionError(type_, data, size);
}

protected:
static constexpr int64_t kMillisPerDay = 86400000;

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();
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();

@ianmcook ianmcook Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 to time64[ns] succeeds, producing 07: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.

@pearu pearu Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
return TimeUnit::SECOND;

@ianmcook ianmcook Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@pearu pearu Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return parsers;
}

TimeUnit::type unit_;
bool expect_timezone_;
static int64_t TicksPerDay(TimeUnit::type unit) {
switch (unit) {
case TimeUnit::SECOND:
return 86400LL;
case TimeUnit::MILLI:
return 86400000LL;
case TimeUnit::MICRO:
return 86400000000LL;
case TimeUnit::NANO:
return 86400000000000LL;
}
return -1; // unreachable
}

const T& concrete_type_;
arrow::internal::StringConverter<T> string_converter_;
const TimeUnit::type parse_unit_;
const int64_t ticks_per_day_;
std::vector<const TimestampParser*> parsers_;
};

Expand Down Expand Up @@ -675,6 +823,27 @@ std::shared_ptr<Converter> MakeTimestampConverter(const std::shared_ptr<DataType
}
}

//
// Concrete Converter factory for dates and times
//

template <template <typename, typename> class ConverterType, typename T>
std::shared_ptr<Converter> MakeDateTimeConverter(const std::shared_ptr<DataType>& type,
const ConvertOptions& options,
MemoryPool* pool,
bool is_type_inference) {
if (is_type_inference || options.timestamp_parsers.empty()) {
// Default to ISO-8601. Type 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.
return std::make_shared<ConverterType<T, NumericValueDecoder<T>>>(type, options,
pool);
}
// Try ISO-8601 first, then the user-defined timestamp parsers
return std::make_shared<ConverterType<T, DateTimeWithParsersValueDecoder<T>>>(
type, options, pool);
}

//
// Concrete Converter factory for reals
//
Expand Down Expand Up @@ -715,7 +884,8 @@ DictionaryConverter::DictionaryConverter(const std::shared_ptr<DataType>& value_

Result<std::shared_ptr<Converter>> Converter::Make(const std::shared_ptr<DataType>& type,
const ConvertOptions& options,
MemoryPool* pool) {
MemoryPool* pool,
bool is_type_inference) {
std::shared_ptr<Converter> ptr;

switch (type->id()) {
Expand Down Expand Up @@ -746,10 +916,6 @@ Result<std::shared_ptr<Converter>> Converter::Make(const std::shared_ptr<DataTyp
NUMERIC_CONVERTER_CASE(Type::FLOAT, FloatType)
NUMERIC_CONVERTER_CASE(Type::DOUBLE, DoubleType)
REAL_CONVERTER_CASE(Type::DECIMAL, Decimal128Type, DecimalValueDecoder)
NUMERIC_CONVERTER_CASE(Type::DATE32, Date32Type)
NUMERIC_CONVERTER_CASE(Type::DATE64, Date64Type)
NUMERIC_CONVERTER_CASE(Type::TIME32, Time32Type)
NUMERIC_CONVERTER_CASE(Type::TIME64, Time64Type)
NUMERIC_CONVERTER_CASE(Type::DURATION, DurationType)
CONVERTER_CASE(Type::BOOL, (PrimitiveConverter<BooleanType, BooleanValueDecoder>))
CONVERTER_CASE(Type::BINARY,
Expand All @@ -763,6 +929,26 @@ Result<std::shared_ptr<Converter>> Converter::Make(const std::shared_ptr<DataTyp
ptr = MakeTimestampConverter<PrimitiveConverter>(type, options, pool);
break;

case Type::DATE32:
ptr = MakeDateTimeConverter<PrimitiveConverter, Date32Type>(type, options, pool,
is_type_inference);
break;

case Type::DATE64:
ptr = MakeDateTimeConverter<PrimitiveConverter, Date64Type>(type, options, pool,
is_type_inference);
break;

case Type::TIME32:
ptr = MakeDateTimeConverter<PrimitiveConverter, Time32Type>(type, options, pool,
is_type_inference);
break;

case Type::TIME64:
ptr = MakeDateTimeConverter<PrimitiveConverter, Time64Type>(type, options, pool,
is_type_inference);
break;

case Type::STRING:
if (options.check_utf8) {
ptr = std::make_shared<PrimitiveConverter<StringType, BinaryValueDecoder<true>>>(
Expand Down
11 changes: 7 additions & 4 deletions cpp/src/arrow/csv/converter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,13 @@ class ARROW_EXPORT Converter {

std::shared_ptr<DataType> type() const { return type_; }

// Create a Converter for the given data type
static Result<std::shared_ptr<Converter>> Make(
const std::shared_ptr<DataType>& type, const ConvertOptions& options,
MemoryPool* pool = default_memory_pool());
// Create a Converter for the given data type.
// If is_type_inference is true, date and time converters ignore
// options.timestamp_parsers and accept only ISO-8601 values.
static Result<std::shared_ptr<Converter>> Make(const std::shared_ptr<DataType>& type,
const ConvertOptions& options,
MemoryPool* pool = default_memory_pool(),
bool is_type_inference = false);

protected:
ARROW_DISALLOW_COPY_AND_ASSIGN(Converter);
Expand Down
Loading
Loading