sunchao commented on code in PR #5932:
URL: https://github.com/apache/datafusion-comet/pull/5932#discussion_r4029599526
##########
native/core/src/parquet/schema_adapter.rs:
##########
@@ -911,6 +1083,51 @@ impl PhysicalExprAdapterFactory for
SparkPhysicalExprAdapterFactory {
None
};
+ // Compare the file's VARIANT annotations against the requested types
before handing the
+ // schemas to the default adapter. `adapted_physical_schema` is used
so that field-id and
+ // case-insensitive resolution has already aligned the two sides'
top-level names.
+ if !self.parquet_options.ignore_variant_annotation {
+ let mut physical_by_folded: HashMap<&str, usize> = HashMap::new();
+ for (i, name) in physical_folded.iter().enumerate() {
+ physical_by_folded.entry(name.as_str()).or_insert(i);
+ }
+ // When the read schema is known, check each requested root
against its requested
+ // type and skip roots Spark would not read. Requested roots share
their Spark names
+ // with the logical file schema, so the same fold pairs them.
+ let required_by_folded: Option<HashMap<String, &FieldRef>> =
+ self.required_schema.as_ref().map(|required| {
+ let mut map = HashMap::new();
+ for (field, folded) in required
+ .fields()
+ .iter()
+ .zip(fold_schema_names(required, case_sensitive))
Review Comment:
### Correctness
[P1] Propagate the fallible fold before building this map
Current `main` changed `fold_schema_names` to return
`DataFusionResult<Vec<String>>` in
[#5845](https://github.com/apache/datafusion-comet/pull/5845). This new call
remains unchanged in the PR's [merge
commit](https://github.com/apache/datafusion-comet/blob/3aa01e9b3cd7408e2d6cf6af3f604e6ef64ac2dc/native/core/src/parquet/schema_adapter.rs#L1109).
Iterating that `Result` yields a `Vec<String>`, so the closure produces
`Option<HashMap<Vec<String>, &FieldRef>>` instead of the declared
`Option<HashMap<String, &FieldRef>>`, and the current merge result cannot
compile. A bounded `rustc` reproduction using the exact added block and folding
function confirms E0308. the same block compiles with the head's older folding
API. Please update this call to propagate the fold error before zipping, and
make the optional-schema closure fallible (for example, return a `Result` and
use `transpose()`), when updating to current `main`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
