You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "rgbuilds (via GitHub)" <gi...@apache.org> on 2026/10/11 04:46:14 UTC
[PR] fix: infer missing nested Parquet fields as nullable [datafusion]
rgbuilds opened a new pull request, #26195:
URL: https://github.com/apache/datafusion/pull/26195
## Which issue does this PR close?
- Closes #26172.
- Depends on #26104. This draft is based on its head (`cf0cf420f4daca37186e748014d896a28855cc74`); the current diff includes that PR. I will rebase onto main after it merges.
## 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 NULL` predicate can incorrectly return no rows. With this change, selecting `s.y` returns `10` and NULL, and `WHERE s.y IS NULL` returns the row from the file without `y`.
## What changes are included in this PR?
Extend partial-field nullability correction recursively through structs, lists, and large lists. Count children only across schemas containing their parent. An absent parent widens the parent; it does not by itself widen its required children. Required list elements remain required while missing struct children within those elements become nullable. Preserve existing nullability and metadata. Other container types retain the existing merge behavior.
Optimized synthetic benchmarks (`release-nonlto`, Apple M4 Mac Mini, in-memory Parquet files, warmed metadata cache, three process runs per variant) against the exact #26104 head measured approximately 2.4–3.0% additional schema-inference time for evolving nested schemas: 32 files × 128 children took 1.973 → 2.033 ms; 256 files × 128 children took 16.072 → 16.459 ms. Flat-schema aggregate medians showed no consistent inference penalty. Registered-table query median differences ranged from -0.15% to +1.12% (queries use universally present columns, since missing-child queries fail on the baseline); these small synthetic measurements do not establish production query impact. The change adds inference traversal and allocations, with no new footer reads or scan-time correction pass.
## 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 NULL` across 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 NULL` row.
- 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.
## Are there any user-facing changes?
Inferred nested fields missing from some containing schemas become nullable, allowing missing values to be read and `IS NULL` queries to return correct rows. Explicit schemas and public APIs are unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: infer missing nested Parquet fields as nullable [datafusion]
Posted by "rgbuilds (via GitHub)" <gi...@apache.org>.
rgbuilds commented on code in PR #26195:
URL: https://github.com/apache/datafusion/pull/26195#discussion_r4291615186
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -272,32 +272,83 @@ fn clear_metadata(
})
}
-/// Marks every field that is missing from at least one of the `file_count`
-/// files as nullable: reading such a file yields nulls for that column.
-fn mark_partially_present_fields_nullable(
- schema: Schema,
- presence: &HashMap<String, usize>,
- file_count: usize,
-) -> Schema {
- if presence.values().all(|&count| count == file_count) {
+/// Widen fields missing from some of their containing schemas. An absent parent
+/// 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 {
Review Comment:
Good catch! I restored a safe fast path that returns when the recursive source field trees are identical, both at the schema root and within unchanged nested levels. I avoided the old top-level-name shortcut because it would hide the missing-child case fixed here.
In a fresh optimized rerun, identical-schema inference improved by 2.92% for 32 x 128 and 0.23% for 256 x 1,024 versus the pre-fast-path version. Evolving nested schemas cannot use the shortcut and retain a small inference cost.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: infer missing nested Parquet fields as nullable [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas merged PR #26195:
URL: https://github.com/apache/datafusion/pull/26195
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: infer missing nested Parquet fields as nullable [datafusion]
Posted by "rgbuilds (via GitHub)" <gi...@apache.org>.
rgbuilds commented on code in PR #26195:
URL: https://github.com/apache/datafusion/pull/26195#discussion_r4291740783
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -272,32 +272,83 @@ fn clear_metadata(
})
}
-/// Marks every field that is missing from at least one of the `file_count`
-/// files as nullable: reading such a file yields nulls for that column.
-fn mark_partially_present_fields_nullable(
- schema: Schema,
- presence: &HashMap<String, usize>,
- file_count: usize,
-) -> Schema {
- if presence.values().all(|&count| count == file_count) {
+/// Widen fields missing from some of their containing schemas. An absent parent
+/// 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 {
return schema;
}
- let metadata = schema.metadata().clone();
- let fields = schema
- .fields()
+ let fields = widen_fields(
+ schema.fields(),
+ &sources.iter().map(Schema::fields).collect::<Vec<_>>(),
+ );
+ Schema::new_with_metadata(fields, schema.metadata().clone())
+}
+
+fn widen_fields(fields: &Fields, sources: &[&Fields]) -> Fields {
+ let mut present: HashMap<&str, Vec<&FieldRef>> = HashMap::new();
+ for source in sources {
+ for field in *source {
+ present.entry(field.name()).or_default().push(field);
+ }
+ }
+ fields
.iter()
.map(|field| {
- let present_everywhere = presence
- .get(field.name())
- .is_some_and(|&count| count == file_count);
- if present_everywhere || field.is_nullable() {
- Arc::clone(field)
+ let occurrences = &present[field.name().as_str()];
+ widen_field(field, occurrences, occurrences.len() < sources.len())
+ })
+ .collect()
+}
+
+fn widen_field(field: &FieldRef, sources: &[&FieldRef], missing: bool) -> FieldRef {
+ let source_types = sources.iter().map(|f| f.data_type()).collect::<Vec<_>>();
+ let data_type = widen_nested_type(field.data_type(), &source_types);
+ let nullable = field.is_nullable() || missing;
+ if &data_type == field.data_type() && nullable == field.is_nullable() {
+ Arc::clone(field)
+ } else {
+ Arc::new(
+ field
+ .as_ref()
+ .clone()
+ .with_data_type(data_type)
+ .with_nullable(nullable),
+ )
+ }
+}
+
+fn widen_nested_type(data_type: &DataType, sources: &[&DataType]) -> DataType {
+ match data_type {
+ DataType::Struct(fields) => {
+ let sources = sources
+ .iter()
+ .filter_map(|t| match t {
+ DataType::Struct(fields) => Some(fields),
+ _ => None,
+ })
+ .collect::<Vec<_>>();
+ DataType::Struct(widen_fields(fields, &sources))
+ }
+ DataType::List(field) | DataType::LargeList(field) => {
+ let sources = sources
+ .iter()
+ .filter_map(|t| match t {
+ DataType::List(f) | DataType::LargeList(f) => Some(f),
+ _ => None,
+ })
+ .collect::<Vec<_>>();
+ let field = widen_field(field, &sources, false);
+ if matches!(data_type, DataType::List(_)) {
+ DataType::List(field)
} else {
- Arc::new(field.as_ref().clone().with_nullable(true))
+ DataType::LargeList(field)
}
- })
- .collect::<Vec<_>>();
- Schema::new_with_metadata(fields, metadata)
+ }
+ // Arrow rejects evolution within other containers before this pass.
Review Comment:
Good point. Addressed in `5bacdb13b`.
I took the second option and made the invariant explicit: unions are not representable in Parquet, while the other child-bearing Arrow types must match exactly during schema merging and therefore cannot reach this pass with nested evolution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: infer missing nested Parquet fields as nullable [datafusion]
Posted by "rgbuilds (via GitHub)" <gi...@apache.org>.
rgbuilds commented on code in PR #26195:
URL: https://github.com/apache/datafusion/pull/26195#discussion_r4291627753
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -272,32 +272,83 @@ fn clear_metadata(
})
}
-/// Marks every field that is missing from at least one of the `file_count`
-/// files as nullable: reading such a file yields nulls for that column.
-fn mark_partially_present_fields_nullable(
- schema: Schema,
- presence: &HashMap<String, usize>,
- file_count: usize,
-) -> Schema {
- if presence.values().all(|&count| count == file_count) {
+/// Widen fields missing from some of their containing schemas. An absent parent
+/// 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 {
return schema;
}
- let metadata = schema.metadata().clone();
- let fields = schema
- .fields()
+ let fields = widen_fields(
+ schema.fields(),
+ &sources.iter().map(Schema::fields).collect::<Vec<_>>(),
+ );
+ Schema::new_with_metadata(fields, schema.metadata().clone())
+}
+
+fn widen_fields(fields: &Fields, sources: &[&Fields]) -> Fields {
+ let mut present: HashMap<&str, Vec<&FieldRef>> = HashMap::new();
+ for source in sources {
+ for field in *source {
+ present.entry(field.name()).or_default().push(field);
+ }
+ }
+ fields
.iter()
.map(|field| {
- let present_everywhere = presence
- .get(field.name())
- .is_some_and(|&count| count == file_count);
- if present_everywhere || field.is_nullable() {
- Arc::clone(field)
+ let occurrences = &present[field.name().as_str()];
Review Comment:
Agreed. I replaced the indexing operation with a guarded lookup and treat an absent occurrence list as missing. I also added a focused unit test confirming the merged field becomes nullable instead of panicking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: infer missing nested Parquet fields as nullable [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #26195:
URL: https://github.com/apache/datafusion/pull/26195#discussion_r4244372355
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -272,32 +272,83 @@ fn clear_metadata(
})
}
-/// Marks every field that is missing from at least one of the `file_count`
-/// files as nullable: reading such a file yields nulls for that column.
-fn mark_partially_present_fields_nullable(
- schema: Schema,
- presence: &HashMap<String, usize>,
- file_count: usize,
-) -> Schema {
- if presence.values().all(|&count| count == file_count) {
+/// Widen fields missing from some of their containing schemas. An absent parent
+/// 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 {
return schema;
}
- let metadata = schema.metadata().clone();
- let fields = schema
- .fields()
+ let fields = widen_fields(
+ schema.fields(),
+ &sources.iter().map(Schema::fields).collect::<Vec<_>>(),
+ );
+ Schema::new_with_metadata(fields, schema.metadata().clone())
+}
+
+fn widen_fields(fields: &Fields, sources: &[&Fields]) -> Fields {
+ let mut present: HashMap<&str, Vec<&FieldRef>> = HashMap::new();
+ for source in sources {
+ for field in *source {
+ present.entry(field.name()).or_default().push(field);
+ }
+ }
+ fields
.iter()
.map(|field| {
- let present_everywhere = presence
- .get(field.name())
- .is_some_and(|&count| count == file_count);
- if present_everywhere || field.is_nullable() {
- Arc::clone(field)
+ let occurrences = &present[field.name().as_str()];
+ widen_field(field, occurrences, occurrences.len() < sources.len())
+ })
+ .collect()
+}
+
+fn widen_field(field: &FieldRef, sources: &[&FieldRef], missing: bool) -> FieldRef {
+ let source_types = sources.iter().map(|f| f.data_type()).collect::<Vec<_>>();
+ let data_type = widen_nested_type(field.data_type(), &source_types);
+ let nullable = field.is_nullable() || missing;
+ if &data_type == field.data_type() && nullable == field.is_nullable() {
+ Arc::clone(field)
+ } else {
+ Arc::new(
+ field
+ .as_ref()
+ .clone()
+ .with_data_type(data_type)
+ .with_nullable(nullable),
+ )
+ }
+}
+
+fn widen_nested_type(data_type: &DataType, sources: &[&DataType]) -> DataType {
+ match data_type {
+ DataType::Struct(fields) => {
+ let sources = sources
+ .iter()
+ .filter_map(|t| match t {
+ DataType::Struct(fields) => Some(fields),
+ _ => None,
+ })
+ .collect::<Vec<_>>();
+ DataType::Struct(widen_fields(fields, &sources))
+ }
+ DataType::List(field) | DataType::LargeList(field) => {
+ let sources = sources
+ .iter()
+ .filter_map(|t| match t {
+ DataType::List(f) | DataType::LargeList(f) => Some(f),
+ _ => None,
+ })
+ .collect::<Vec<_>>();
+ let field = widen_field(field, &sources, false);
+ if matches!(data_type, DataType::List(_)) {
+ DataType::List(field)
} else {
- Arc::new(field.as_ref().clone().with_nullable(true))
+ DataType::LargeList(field)
}
- })
- .collect::<Vec<_>>();
- Schema::new_with_metadata(fields, metadata)
+ }
+ // Arrow rejects evolution within other containers before this pass.
Review 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.
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -272,32 +272,83 @@ fn clear_metadata(
})
}
-/// Marks every field that is missing from at least one of the `file_count`
-/// files as nullable: reading such a file yields nulls for that column.
-fn mark_partially_present_fields_nullable(
- schema: Schema,
- presence: &HashMap<String, usize>,
- file_count: usize,
-) -> Schema {
- if presence.values().all(|&count| count == file_count) {
+/// Widen fields missing from some of their containing schemas. An absent parent
+/// 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 {
Review 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`.
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -272,32 +272,83 @@ fn clear_metadata(
})
}
-/// Marks every field that is missing from at least one of the `file_count`
-/// files as nullable: reading such a file yields nulls for that column.
-fn mark_partially_present_fields_nullable(
- schema: Schema,
- presence: &HashMap<String, usize>,
- file_count: usize,
-) -> Schema {
- if presence.values().all(|&count| count == file_count) {
+/// Widen fields missing from some of their containing schemas. An absent parent
+/// 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 {
return schema;
}
- let metadata = schema.metadata().clone();
- let fields = schema
- .fields()
+ let fields = widen_fields(
+ schema.fields(),
+ &sources.iter().map(Schema::fields).collect::<Vec<_>>(),
+ );
+ Schema::new_with_metadata(fields, schema.metadata().clone())
+}
+
+fn widen_fields(fields: &Fields, sources: &[&Fields]) -> Fields {
+ let mut present: HashMap<&str, Vec<&FieldRef>> = HashMap::new();
+ for source in sources {
+ for field in *source {
+ present.entry(field.name()).or_default().push(field);
+ }
+ }
+ fields
.iter()
.map(|field| {
- let present_everywhere = presence
- .get(field.name())
- .is_some_and(|&count| count == file_count);
- if present_everywhere || field.is_nullable() {
- Arc::clone(field)
+ let occurrences = &present[field.name().as_str()];
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: infer missing nested Parquet fields as nullable [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26195:
URL: https://github.com/apache/datafusion/pull/26195#issuecomment-6105709833
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26195?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `93.38235%` with `9 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.81%. Comparing base ([`102a162`](https://app.codecov.io/gh/apache/datafusion/commit/102a1628592121d96bfe3d09d80e63180eef288a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`278b3e5`](https://app.codecov.io/gh/apache/datafusion/commit/278b3e55c6af9f61b333c5024791e14435bc998e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 35 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/26195?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/datasource-parquet/src/file\_format.rs](https://app.codecov.io/gh/apache/datafusion/pull/26195?src=pr&el=tree&filepath=datafusion%2Fdatasource-parquet%2Fsrc%2Ffile_format.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9kYXRhc291cmNlLXBhcnF1ZXQvc3JjL2ZpbGVfZm9ybWF0LnJz) | 93.38% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26195?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #26195 +/- ##
==========================================
+ Coverage 82.74% 82.81% +0.06%
==========================================
Files 1147 1149 +2
Lines 449767 452341 +2574
Branches 449767 452341 +2574
==========================================
+ Hits 372157 374599 +2442
- Misses 54942 54944 +2
- Partials 22668 22798 +130
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26195?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: infer missing nested Parquet fields as nullable [datafusion]
Posted by "rgbuilds (via GitHub)" <gi...@apache.org>.
rgbuilds commented on PR #26195:
URL: https://github.com/apache/datafusion/pull/26195#issuecomment-6106058897
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: infer missing nested Parquet fields as nullable [datafusion]
Posted by "rgbuilds (via GitHub)" <gi...@apache.org>.
rgbuilds commented on code in PR #26195:
URL: https://github.com/apache/datafusion/pull/26195#discussion_r4291627753
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -272,32 +272,83 @@ fn clear_metadata(
})
}
-/// Marks every field that is missing from at least one of the `file_count`
-/// files as nullable: reading such a file yields nulls for that column.
-fn mark_partially_present_fields_nullable(
- schema: Schema,
- presence: &HashMap<String, usize>,
- file_count: usize,
-) -> Schema {
- if presence.values().all(|&count| count == file_count) {
+/// Widen fields missing from some of their containing schemas. An absent parent
+/// 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 {
return schema;
}
- let metadata = schema.metadata().clone();
- let fields = schema
- .fields()
+ let fields = widen_fields(
+ schema.fields(),
+ &sources.iter().map(Schema::fields).collect::<Vec<_>>(),
+ );
+ Schema::new_with_metadata(fields, schema.metadata().clone())
+}
+
+fn widen_fields(fields: &Fields, sources: &[&Fields]) -> Fields {
+ let mut present: HashMap<&str, Vec<&FieldRef>> = HashMap::new();
+ for source in sources {
+ for field in *source {
+ present.entry(field.name()).or_default().push(field);
+ }
+ }
+ fields
.iter()
.map(|field| {
- let present_everywhere = presence
- .get(field.name())
- .is_some_and(|&count| count == file_count);
- if present_everywhere || field.is_nullable() {
- Arc::clone(field)
+ let occurrences = &present[field.name().as_str()];
Review Comment:
Agreed. I replaced the indexing operation with a guarded lookup and treat an absent occurrence list as missing. I also added a focused unit test confirming the merged field becomes nullable instead of panicking.
The duplicate-name point is valid and also affects the pre-existing top-level counting. Correctly consolidating duplicate fields within one source has broader semantics than this nested-nullability fix, so I left it unchanged and propose tracking it separately unless you consider it blocking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: infer missing nested Parquet fields as nullable [datafusion]
Posted by "rgbuilds (via GitHub)" <gi...@apache.org>.
rgbuilds commented on PR #26195:
URL: https://github.com/apache/datafusion/pull/26195#issuecomment-6209962471
@zhuqi-lucas Thanks for the detailed review. I addressed the three actionable points in `c14317f1f` and `5bacdb13b`:
- Restored a safe recursive-equality fast path for identical schemas and unchanged nested levels.
- Replaced the unguarded presence-map lookup with a defensive lookup and added a focused regression test.
- Clarified the container-type invariant by naming the Arrow types whose nested evolution is rejected during schema merging.
The duplicate-name issue is valid and also affects the pre-existing top-level logic. Correctly consolidating duplicate fields within a single source has broader semantics than this fix, so I left it unchanged and suggest tracking it separately unless you consider it blocking.
I also reran the optimized schema-inference benchmark. Against current `upstream/main`, identical schemas were 2.78–4.99% faster, while evolving nested schemas were 2.11–6.07% slower, adding approximately 0.041–0.947 ms in the tested cases.
Formatting and the modified-crate Clippy checks pass. The focused unit, Parquet integration, and SQL logic tests also pass.
Ready for another look when convenient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org