Repository navigation
fix: reject duplicate field names during CSV and Parquet schema inference - #24387
Merged
alamb merged 4 commits intoAug 20, 2026
Merged
Conversation
github-actions
Bot
added
core
Jefffrey
approved these changes
Aug 16, 2026
Jefffrey
left a comment
Jefffrey
left a comment
Contributor
There was a problem hiding this comment.
this makes sense to me; i hope its a niche enough bug to not cause too much issues for downstream users now that itll error, though i suppose the other solution of renaming the duplicates would be breaking too anyway
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24387 +/- ##
==========================================
- Coverage 81.31% 81.30% -0.01%
==========================================
Files 1117 1117
Lines 395911 395980 +69
Branches 395911 395980 +69
==========================================
+ Hits 321918 321969 +51
- Misses 55177 55185 +8
- Partials 18816 18826 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
Yep, that was my reasoning too. This seemed safer than renaming the duplicates, and silently dropping columns is the worst option imo. In the case of downstream issues, I’d be cool with following up. Ty. |
alamb
approved these changes
Aug 20, 2026
| /// [`Schema::try_merge`] coalesces fields by name, so callers validate each | ||
| /// inferred file schema before merging. | ||
| pub fn ensure_unique_field_names(schema: &Schema) -> Result<()> { | ||
| let mut seen = HashSet::with_capacity(schema.fields().len()); |
Contributor
There was a problem hiding this comment.
this is going to make a hashset for schema, -- maybe can we instead reuse the same hash table?
Contributor
Author
There was a problem hiding this comment.
Done (cbdd34f). CSV and Parquet now reuse the same HashSet across the schemas in one inference call. It gets cleared between schemas and keeps the allocation. I also kept the CSV paths borrowed, so this doesn't add a path clone per file.
alamb
reviewed
Aug 20, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thank you for this @sovsparrow and @Jefffrey
It would be better I think to have an end to end toest (aka a slt test that reads a csv file with duplicate column names) to make sure this is hooked up end to end correctly
Is there any chance we can add that too?
alamb
approved these changes
Aug 20, 2026
alamb
left a comment
alamb
left a comment
Contributor
There was a problem hiding this comment.
Thank you for this @sovsparrow and @Jefffrey
It would be better I think to have an end to end toest (aka a slt test that reads a csv file with duplicate column names) to make sure this is hooked up end to end correctly
Is there any chance we can add that too?
Contributor
|
run benchmark sql_planner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing fix/duplicate-field-name-inference (9d30ff9) to 47e4765 (merge-base) diff Run configurationrun benchmark sql_plannerResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing fix/duplicate-field-name-inference (9d30ff9) to 47e4765 (merge-base) diff Run configurationrun benchmark sql_plannerCPU Details (lscpu)Details
Resource Usagesql_planner — base (merge-base)
sql_planner — branch
File an issue against this benchmark runner |
Contributor
Author
|
@alamb I added this to |
alamb
enabled auto-merge
August 20, 2026 17:42
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
When a Parquet file has multiple columns with the same name and compatible types, DataFusion merges them during schema inference. The scan succeeds without warning but returns only one of those columns.
PyArrow can produce such a file via its public API:
DataFusion 54.0.0 returns ['id', 'value'] for this file; the [100, 200, 300] column is missing.
Other readers either preserve the data or reject the file:
pyarrowParquetFile.read()pyarrowpq.read_table()ArrowInvalid: Multiple matches for FieldRef.Name(value)pyarrowdatasetArrowInvalid: Can't unify schema with duplicate field namesduckdbvalue_1polarsDuplicateErrordatafusionThe same bug is behind two issues #24381 and #12852. CSV and Parquet schema inference both pass each file's schema to
Schema::try_merge, which matches fields by name. Duplicate names in one file are merged before the scan. Thus, column(s) disappear silently.What changes are included in this PR?
This PR adds
ensure_unique_field_namestodatafusion-datasource. It is called for each inferred CSV and Parquet schema before the merge. It uses the existingSchemaError::DuplicateUnqualifiedFielderror and adds the file location to the message.Duplicate names are rejected. The PR does not rename columns or change how fields with unique names are merged across files.
Are these changes tested?
Yes. Two regression tests in order to cover the CSV and Parquet paths:
datafusion/core/src/datasource/file_format/parquet.rs—infer_schema_rejects_duplicate_field_namesdatafusion/core/src/datasource/file_format/csv.rs—infer_schema_rejects_duplicate_header_namesBoth tests create their inputs in a temporary directory; no fixtures are added. Both fail on
mainand pass with this change. Onmain, the CSV test infersSchema { fields: [id, value] }from a three-column header, matching the behavior reported in #12852.Are there any user-facing changes?
Yes. CSV and Parquet schema inference now returns a clear error when an inferred
file schema repeats a field name. The error names the file and the repeated
column instead of returning an incomplete schema.