comphead commented on code in PR #5262:
URL: https://github.com/apache/datafusion-comet/pull/5262#discussion_r3935620815
##########
native/core/src/parquet/schema_adapter.rs:
##########
@@ -77,6 +78,91 @@ fn schema_has_field_ids(schema: &SchemaRef) -> bool {
schema.fields().iter().any(|f| parse_field_id(f).is_some())
}
+/// Returns true when casting `physical_type` to `target_type` is a *pure*
structural
+/// narrowing (dropping unrequested struct/list fields, no leaf-level value
reinterpretation)
+/// that DataFusion's own `datafusion_common::nested_struct::cast_column`
already computes
+/// identically to Comet's `spark_parquet_convert`. `ColumnarValue::cast_to`
(the function a
+/// plain, un-swapped `CastExpr` runs at execution time; see
+/// datafusion/expr-common/src/columnar_value.rs) routes to that same function
whenever
+/// `datafusion_common::nested_struct::requires_nested_struct_cast` holds,
matching arbitrary
+/// struct/list fields by name, null-filling missing target fields, and
dropping extra source
+/// fields, exactly the shape apache/datafusion-comet#4859 needs pruned.
Confirmed
+/// byte-identical to `spark_parquet_convert` for the covered shapes by
+/// `test::nested_struct_narrowing_cast_matches_datafusion_generic_cast`.
+///
+/// When this returns `true`, `replace_with_spark_cast` leaves DataFusion's
`CastExpr` in
+/// place instead of swapping in `CometCastColumnExpr`, so DataFusion's
leaf-pruning
+/// (`build_projection_read_plan`'s cast-clipping, apache/datafusion#24090)
can see the cast
+/// and read only the requested Parquet leaves, instead of falling back to a
full-column read
+/// because it can't recognize `CometCastColumnExpr`.
+///
+/// This is deliberately an allow list, not a deny list: it only recurses
through the two
+/// container shapes `nested_struct::cast_column` actually implements (Struct,
List /
+/// LargeList), and requires every leaf it bottoms out at to be an *exact*
type match. Pruning
+/// (the only case this predicate needs to cover, see
apache/datafusion-comet#4859) only
+/// changes which struct/list fields are kept, never a leaf's type, so
exact-match leaves are
+/// sufficient. A deny list here (enumerate every case where Comet's nested
cast differs from
+/// Arrow's, allow everything else) would fail open: a future addition to
+/// `parquet_convert_array` that this predicate does not know to also exclude
would silently
+/// start producing wrong results instead of just missing an optimization.
+fn is_pure_structural_narrowing(
+ physical_type: &DataType,
+ target_type: &DataType,
+ parquet_options: &SparkParquetOptions,
+) -> bool {
+ match (physical_type, target_type) {
+ (DataType::Struct(source_fields), DataType::Struct(target_fields)) => {
+ // Comet matches by Parquet field id first when the target carries
one;
+ // DataFusion's generic cast has no field-id concept, so any
field-id-bearing
+ // target field is a potential divergence.
+ if parquet_options.use_field_id
+ && target_fields.iter().any(|f| parse_field_id(f).is_some())
+ {
+ return false;
+ }
+ target_fields.iter().all(|target_field| {
+ // Require an *exact* (case-sensitive) name match for every
target field.
+ // `nested_struct::cast_column` always matches by exact name;
Comet
+ // additionally matches case-insensitively when
`case_sensitive` is false,
+ // which could resolve a field DataFusion would instead treat
as missing (and
+ // null-fill). Requiring an exact match sidesteps that
divergence regardless
+ // of the `case_sensitive` setting, and also sidesteps the
missing-field
+ // nullability divergence: DataFusion errors when a
non-nullable target field
+ // is missing from the source, whereas Comet null-fills
unconditionally.
+ source_fields
+ .iter()
+ .find(|f| f.name() == target_field.name())
+ .is_some_and(|source_field| {
Review Comment:
it might be a good follow up ticket
--
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]
