fix: apply CSV null_regex when reading, not only when inferring the schema - #25254
Open
Developer1010x wants to merge 1 commit into
Open
Developer1010x wants to merge 1 commit into
Developer1010x wants to merge 1 commit into
Conversation
…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>
This was referenced Sep 13, 2026
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
Setting
null_regexon 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:
That second case is the one that matters in practice —
N/A,NULLand-placeholders are almost always sitting in columns that are otherwise numeric,
which is the reason to reach for
null_regexin the first place.The option was applied to the
arrow::csv::reader::Formatused byCsvFormat::infer_schema, but never to theReaderBuilderthat parses therows. 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 existingescape(),comment()andterminator()accessors.CsvSource::builder()applies the regex, and now returnsResultso aninvalid pattern surfaces as a configuration error instead of panicking.
CsvFormat::infer_schemacurrently.expect()s on the same regex, so amalformed pattern is a panic there today; this side no longer adds a second
one.
CsvSource::open, and the twostreaming decoder paths in
CsvOpener::open(the byte-range branch and theGetResultPayload::Streambranch). Those two go throughbuilder()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_regexanywhere in the repository before thischange — not in
datafusion/datasource-csv, not in the sqllogictest files.Unit tests in
datafusion/datasource-csv/src/source.rs:null_regex_nulls_matching_string_valuesnull_regex_nulls_matching_values_in_numeric_columns— the case that errorstoday rather than returning a wrong value
without_null_regex_the_placeholder_is_read_verbatim— control, pins that theunset behavior does not change
invalid_null_regex_is_reported_as_an_errorI 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 anew
datafusion/core/tests/data/null_regex.csvfixture, covering a matchingvalue in both a
VARCHARand anINTcolumn.Are there any user-facing changes?
null_regexstarts doing what it is documented to do. Anything that set theoption and worked around it being ignored — for example by typing a column as
VARCHARto 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 newnull_regex()accessor is additive.