Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26195 +/- ##
==========================================
- Coverage 82.80% 82.80% -0.01%
==========================================
Files 1149 1149
Lines 452234 452347 +113
Branches 452234 452347 +113
==========================================
+ Hits 374493 374581 +88
- Misses 54946 54969 +23
- Partials 22795 22797 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@zhuqi-lucas This builds on #26104 and addresses the nested-field case we discussed, tracked in #26172. The fix and tests are in the final commit, 278b3e5; local validation and CI passed. Would you take a look when convenient? I’ll rebase once #26104 merges. I’ve included schema-inference benchmarks in the description and would appreciate feedback on the tradeoff. |
278b3e5 to
0c0fe4c
Compare
| .collect::<Vec<_>>(); | ||
| Schema::new_with_metadata(fields, metadata) | ||
| } | ||
| // Arrow rejects evolution within other containers before this pass. |
There was a problem hiding this comment.
This comment is not accurate for Union. Field::try_merge in arrow-schema/src/field.rs recurses into Struct, Union, List and LargeList; Map and FixedSizeList fall into the equality arm, so the comment holds for those. A union whose child is missing from some files merges fine and then lands here, where it is silently left alone, which is the same bug one container away.
Low risk in practice since Parquet cannot carry a union, but the comment states an invariant that does not hold, and the next person will rely on it. Either widen unions too, or name the types that are actually rejected.
| /// produces a null parent, not a null value for each required child, so children | ||
| /// are compared only across files in which their parent exists. | ||
| fn mark_partially_present_fields_nullable(schema: Schema, sources: &[Schema]) -> Schema { | ||
| if sources.len() <= 1 { |
There was a problem hiding this comment.
The old fast path is gone. It returned early when every field was present in every file, using the precomputed counts, which is the common case of all files sharing a schema. Now only the single-source case returns early, so identical schemas still walk the whole tree, build a HashMap per struct level and allocate a fresh Fields per level. That looks like the 2.4-3.0% the description measures.
Cheap to restore: compare the field name sets across sources first, or short-circuit a level when every widen_field returned the same Arc.
| .is_some_and(|&count| count == file_count); | ||
| if present_everywhere || field.is_nullable() { | ||
| Arc::clone(field) | ||
| let occurrences = &present[field.name().as_str()]; |
There was a problem hiding this comment.
Unguarded map index, so a missing key panics rather than erroring. I could not construct a reachable case, since a merged field name always comes from some source, but that leans on a try_merge invariant instead of enforcing it here. .get() with an empty slice treated as missing is both safer and the semantically right default.
Separately, and this predates the PR so take it or leave it: the loop above pushes per field rather than per source, so a file carrying two fields of the same name contributes two occurrences. occurrences.len() < sources.len() can then be false even though another file lacks the field, leaving it required. Counting distinct sources would fix it.
Which issue does this PR close?
Rationale for this change
When a required child is missing from one Parquet file but its containing struct exists in both, inference leaves the child non-nullable. Selecting the child fails, and an
IS NULLpredicate can incorrectly return no rows. With this change, selectings.yreturns10and NULL, andWHERE s.y IS NULLreturns the row from the file withouty.What changes are included in this PR?
Implementation
Performance
release-nonlto, in-memory Parquet files, warmed metadata, and three process runs per variant. These synthetic results do not establish production query impact.What is the testing strategy for this PR?
Unit tests cover absent parents, independent nested paths, metadata, and required list elements.
A Parquet integration regression checks struct/list/large-list projection and
IS NULLacross 12 configurations (metadata and pushdown on/off). It fails on the unfixed dependency head.SQL regression verifies the missing child's projected NULL and the matching
IS NULLrow.Fresh extended workspace validation passed all Rust library/integration/bin tests and all 529 SQL logic files. The 126 extended fuzz tests passed in the initial run; subsequent Rust runs skipped only those already-passing cases. All-target/all-feature Clippy with warnings denied, formatting, and every submission lint check passed. macOS emitted linker unwind-table warnings when linking large test executables.
After rebasing onto main following the merge of fix: infer a Parquet column as nullable when some files lack it #26104: formatting, the full submission lint suite (including CI-configured Clippy), and extended workspace Rust/SQL tests passed. The all-target/all-feature Clippy command reports three
mutable_key_typeerrors in unchanged upstream optimizer code.Are there any user-facing changes?
Inferred nested fields missing from some containing schemas become nullable, allowing missing values to be read and
IS NULLqueries to return correct rows. Explicit schemas and public APIs are unchanged.