Skip to content

fix: return an error for an invalid CSV null_regex instead of panicking - #25261

Merged
Jefffrey merged 1 commit into
apache:mainfrom
Developer1010x:csv-null-regex-error-not-panic
Sep 15, 2026
Merged

Jefffrey merged 1 commit into
apache:mainfrom
Developer1010x:csv-null-regex-error-not-panic

Conversation

@Developer1010x

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

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

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:

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 #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 #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.

`CsvFormat::infer_schema_from_stream` compiled the pattern with
`Regex::new(..).expect("Unable to parse CSV null regex.")`, so a pattern the
regex crate rejects aborted the query task rather than surfacing as a
`DataFusionError`. It is reachable from SQL through any `CREATE EXTERNAL TABLE`
that leaves its columns to schema inference:

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

Compile the regex once before the chunk loop and propagate a failure, which
also stops the pattern being recompiled for every chunk.

Tests: a `statement error` case in `csv_files.slt`. It reproduces the panic
without this change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) datasource Changes to the datasource crate labels Sep 13, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.88%. Comparing base (9082d6b) to head (a02b612).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #25261    +/-   ##
========================================
  Coverage   81.88%   81.88%            
========================================
  Files        1133     1133            
  Lines      424522   424668   +146     
  Branches   424522   424668   +146     
========================================
+ Hits       347623   347754   +131     
- Misses      56285    56296    +11     
- 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.

@Jefffrey
Jefffrey added this pull request to the merge queue Sep 15, 2026
@Jefffrey

Copy link
Copy Markdown
Contributor

thanks @Developer1010x

Merged via the queue into apache:main with commit 98d8412 Sep 15, 2026
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid CSV null_regex panics the query task instead of returning an error

3 participants