Repository navigation
fix: infer a Parquet column as nullable when some files lack it - #26104
zhuqi-lucas wants to merge 3 commits into
Conversation
ListingTable schema inference merges per-file schemas with Schema::try_merge, which only widens nullability for fields present in more than one file. A column that exists, as required, in some files and not at all in others therefore came out NOT NULL, and reading the files without it failed with "declared as non-nullable but contains null values". Track which files carry each field and mark fields that are missing from any file nullable after the merge.
a0d90a8 to
872bbd9
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26104 +/- ##
==========================================
+ Coverage 82.74% 82.76% +0.02%
==========================================
Files 1147 1147
Lines 449767 450603 +836
Branches 449767 450603 +836
==========================================
+ Hits 372157 372953 +796
+ Misses 54942 54938 -4
- Partials 22668 22712 +44 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟢 Approval recommended
The implementation directly addresses the reported failure and includes comprehensive unit and end-to-end regression coverage.
0 open findings
What changed in this PR
Fixes Parquet schema inference so columns absent from some files are nullable, preventing scan failures during schema evolution.
Changes:
- Tracks top-level field presence across Parquet files and adjusts merged nullability.
- Adds unit coverage for reordered, required, nullable, and missing columns.
- Adds an end-to-end SQL logic regression test.
| File | Description |
|---|---|
datafusion/datasource-parquet/src/file_format.rs |
Corrects inferred nullability for partially present fields. |
datafusion/core/tests/parquet/schema.rs |
Tests schema inference and reads across evolving files. |
datafusion/sqllogictest/test_files/schema_evolution.slt |
Verifies SQL-visible schema and query results. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| ---- | ||
| 1 10 | ||
| 2 NULL | ||
|
|
There was a problem hiding this comment.
Could you please consider adding SELECT id FROM inferred_partial WHERE c IS NULL, expecting 2, here? This would also cover the optimizer behavior: the base incorrectly returns no rows, while this fix returns the expected row.
There was a problem hiding this comment.
The struct child case is a separate hole, presence is only tracked at the top level here; please do open an issue with your reproducer, or i can help create the issue.
There was a problem hiding this comment.
Thanks for adding the test and confirming the nested-field case! I’ll open a separate issue with the reproducer and investigate further.
|
I also reproduced a related case where both files contain struct Would it be useful to track this separately? I’d be happy to open an issue with the reproducer and investigate further. |
rgbuilds
left a comment
There was a problem hiding this comment.
Thanks for the fix! It works locally and Rust/SQL tests pass. LGTM.
I’ve included an optional test suggestion and a related follow-up observation for your consideration.
Which issue does this PR close?
Rationale for this change
ListingTableschema inference over Parquet merges the per-file schemas withSchema::try_merge, which widens nullability only for fields that appear in more than one schema. A column that isrequiredin some files and absent from the others therefore comes outNOT NULL, but reading the files without it fills it with nulls, and the scan fails:Pure SQL reproducer (on
main):Before DataFusion 52 this was masked by
SchemaAdapter::map_batch, which rebuilt batches under the table schema without validating nullability (removed in #18998). The strict check is right; the inferred schema is what is wrong.What changes are included in this PR?
In
ParquetFormat::infer_schema, count in how many files each top-level field appears and, after the merge, mark every field that is missing from at least one file as nullable. Fields present in every file keep whatevertry_mergeproduced, so tables whose files all share a schema are unchanged.Are these changes tested?
schema_evolution.sltcase: twoCOPY TOfiles,CREATE EXTERNAL TABLEwithout a schema,DESCRIBEshows the partial column as nullable,SELECTreturns the null. Fails onmainwith the error above.schema_merge_marks_partially_present_columns_nullableincore/tests/parquet/schema.rs: three files, column in a different position, an already-nullable partial column, and bothskip_metadatapaths.Are there any user-facing changes?
An inferred Parquet table schema now reports a column as nullable when some files do not contain it. Tables where every file has every column are unaffected. Explicitly declared schemas are not touched.