Skip to content

fix: apply CSV null_regex when reading, not only when inferring the schema - #25254

Open
Developer1010x wants to merge 1 commit into
apache:mainfrom
Developer1010x:csv-apply-null-regex-to-reader
Open

Developer1010x wants to merge 1 commit into
apache:mainfrom
Developer1010x:csv-apply-null-regex-to-reader

Conversation

@Developer1010x

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Setting null_regex on a CSV source has no effect on the data that comes back.
A field matching the pattern is read as the literal string, and if it lands in a
column typed as a number the query fails outright:

Arrow error: Parser error: Error while parsing value 'N/A' as type 'Int64'
for column 1 at line 2. Row data: '[2,N/A]'

That second case is the one that matters in practice — N/A, NULL and -
placeholders are almost always sitting in columns that are otherwise numeric,
which is the reason to reach for null_regex in the first place.

The option was applied to the arrow::csv::reader::Format used by
CsvFormat::infer_schema, but never to the ReaderBuilder that parses the
rows. Inference therefore behaves as though the regex were honored and only the
data disagrees, which is what makes it easy to miss.

What changes are included in this PR?

  • CsvSource::null_regex(), alongside the existing escape(), comment() and
    terminator() accessors.
  • CsvSource::builder() applies the regex, and now returns Result so an
    invalid pattern surfaces as a configuration error instead of panicking.
    CsvFormat::infer_schema currently .expect()s on the same regex, so a
    malformed pattern is a panic there today; this side no longer adds a second
    one.
  • Three call sites updated for the new signature: CsvSource::open, and the two
    streaming decoder paths in CsvOpener::open (the byte-range branch and the
    GetResultPayload::Stream branch). Those two go through builder() as well,
    so they were missing the regex for the same reason.

The fix is four lines; the signature change is what makes it touch more.

What is the testing strategy for this PR?

There was no coverage of null_regex anywhere in the repository before this
change — not in datafusion/datasource-csv, not in the sqllogictest files.

Unit tests in datafusion/datasource-csv/src/source.rs:

  • null_regex_nulls_matching_string_values
  • null_regex_nulls_matching_values_in_numeric_columns — the case that errors
    today rather than returning a wrong value
  • without_null_regex_the_placeholder_is_read_verbatim — control, pins that the
    unset behavior does not change
  • invalid_null_regex_is_reported_as_an_error

I checked these are not vacuous: with the four added lines in builder()
removed, the three behavioral tests fail and the control still passes.

SQL-level coverage in datafusion/sqllogictest/test_files/csv_files.slt, over a
new datafusion/core/tests/data/null_regex.csv fixture, covering a matching
value in both a VARCHAR and an INT column.

Are there any user-facing changes?

null_regex starts doing what it is documented to do. Anything that set the
option and worked around it being ignored — for example by typing a column as
VARCHAR to avoid the parse error, then filtering the placeholder out in SQL —
will now see NULL where it previously saw the literal string.

No public API changes: builder() is private, and the new null_regex()
accessor is additive.

…chema

`CsvOptions::null_regex` was set on the `arrow::csv::reader::Format` used by
`CsvFormat::infer_schema`, but `CsvSource::builder` never passed it to the
`ReaderBuilder` that parses the rows. A field matching the regex was therefore
read as the literal string, and in a column typed as a number the read failed
with an Arrow parser error instead of producing NULL:

    Arrow error: Parser error: Error while parsing value 'N/A' as type 'Int64'
    for column 1 at line 2. Row data: '[2,N/A]'

`builder` now applies the regex, and returns `Result` so an invalid pattern is
reported as a configuration error rather than panicking. Both the file reader
and the two streaming decoder paths go through `builder`, so all three are
covered.

Tests: four unit tests in `datafusion/datasource-csv/src/source.rs` covering a
string column, a numeric column, the unset case, and an invalid pattern; plus
sqllogictest coverage in `csv_files.slt`. The three behavioral tests fail
without this change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.73913% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.88%. Comparing base (9082d6b) to head (7ac3811).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/datasource-csv/src/source.rs 96.73% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #25254   +/-   ##
=======================================
  Coverage   81.88%   81.88%           
=======================================
  Files        1133     1133           
  Lines      424522   424609   +87     
  Branches   424522   424609   +87     
=======================================
+ Hits       347623   347701   +78     
- Misses      56285    56290    +5     
- Partials    20614    20618    +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

zhuqi-lucas pushed a commit to zhuqi-lucas/arrow-datafusion that referenced this pull request Sep 15, 2026
…ng (apache#25261)

## Which issue does this PR close?

- Closes apache#25260.

## Rationale for this change

A malformed `null_regex` panics the query task instead of returning an
error.
`CsvFormat::infer_schema_from_stream` compiled the pattern with

```rust
let regex = Regex::new(null_regex.as_str())
    .expect("Unable to parse CSV null regex.");
```

so any pattern the `regex` crate rejects aborted the task. It is
reachable
straight from SQL, through a `CREATE EXTERNAL TABLE` that leaves its
columns to
schema inference:

```sql
CREATE EXTERNAL TABLE t STORED AS CSV LOCATION 'data.csv'
OPTIONS ('format.has_header' 'true', 'format.null_regex' '(');
```

```
task 9 panicked with message "Unable to parse CSV null regex.: Syntax(
regex parse error:
    (
    ^
error: unclosed group
)"
```

An invalid regex is a bad option value, not an internal invariant, so it
should
come back as an error naming the pattern.

## What changes are included in this PR?

The regex is compiled once, before the per-chunk loop, and a failure is
propagated with `exec_datafusion_err!` rather than panicking. Hoisting
it also
stops the pattern being recompiled for every chunk of the inference
stream.

The error text matches the one used on the read side in apache#25254, so the
same bad
option reads the same whichever path hits it first.

## What is the testing strategy for this PR?

A `statement error Unable to parse CSV null regex` case in
`datafusion/sqllogictest/test_files/csv_files.slt`.

I checked it is not vacuous: with this change reverted, that case fails
with the
panic quoted above rather than an error, so the test reproduces the bug.

`cargo fmt --check`, `cargo clippy --all-targets` and the crate's unit
tests are
clean.

## Are there any user-facing changes?

An invalid `null_regex` now produces an error and leaves the session
usable,
where it previously panicked the task. No API changes.

Independent of apache#25254 — that one is the read path in `source.rs`, this
is the
inference path in `file_format.rs` — but they touch the same crate, so
whichever
lands second may want a trivial rebase.

Co-authored-by: Prajwal Narayana <sprajwalln@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate datasource Changes to the datasource crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CSV null_regex is applied to schema inference but never to the reader, so matching values are not null

2 participants