martin-g commented on code in PR #25633:
URL: https://github.com/apache/datafusion/pull/25633#discussion_r4082217630
##########
datafusion/datasource-parquet/src/projection_read_plan.rs:
##########
@@ -521,6 +549,22 @@ impl TreeNodeVisitor<'_> for PushdownChecker<'_> {
}
}
+/// The field path from a Struct down to its first non-Struct child, or `None`
+/// when `data_type` is not a Struct or no child has a leaf.
+fn first_leaf_path(data_type: &DataType) -> Option<Vec<String>> {
Review Comment:
The function name is misleading. As the docstring says it returns the path
to the first non-struct field. It is not necessary to be a leaf - it could be a
List, Map, Union, ...
##########
datafusion/datasource-parquet/src/projection_read_plan.rs:
##########
@@ -521,6 +549,22 @@ impl TreeNodeVisitor<'_> for PushdownChecker<'_> {
}
}
+/// The field path from a Struct down to its first non-Struct child, or `None`
+/// when `data_type` is not a Struct or no child has a leaf.
+fn first_leaf_path(data_type: &DataType) -> Option<Vec<String>> {
+ let DataType::Struct(fields) = data_type else {
+ return None;
+ };
+
+ fields.iter().find_map(|field| {
+ let mut path = vec![field.name().clone()];
+ if matches!(field.data_type(), DataType::Struct(_)) {
+ path.extend(first_leaf_path(field.data_type())?);
+ }
+ Some(path)
+ })
Review Comment:
This could be optimized by checking first the primitive types:
```suggestion
fields
.iter()
.filter(|field| !field.data_type().is_nested())
.chain(
fields
.iter()
.filter(|field| field.data_type().is_nested()),
)
.find_map(|field| {
let mut path = vec![field.name().clone()];
if matches!(field.data_type(), DataType::Struct(_)) {
path.extend(first_leaf_path(field.data_type())?);
}
Some(path)
})
```
##########
datafusion/datasource-parquet/src/projection_read_plan.rs:
##########
@@ -1982,6 +2026,53 @@ mod test {
}
}
+ #[test]
+ fn first_leaf_path_descends_to_the_first_primitive() {
+ let inner = DataType::Struct(
+ vec![
+ Arc::new(Field::new("inner", DataType::Int32, true)),
+ Arc::new(Field::new("other", DataType::Int32, true)),
+ ]
+ .into(),
+ );
+ let outer = DataType::Struct(
+ vec![
+ Arc::new(Field::new("outer", inner, true)),
+ Arc::new(Field::new("tag", DataType::Utf8, true)),
+ ]
+ .into(),
+ );
+
+ assert_eq!(
+ first_leaf_path(&outer),
+ Some(vec!["outer".to_string(), "inner".to_string()])
+ );
+ // A non-Struct child ends the path; its own leaves are what gets read.
+ let list_first = DataType::Struct(
+ vec![Arc::new(Field::new_list(
+ "items",
+ Field::new("item", DataType::Int32, true),
+ true,
+ ))]
+ .into(),
+ );
+ assert_eq!(
+ first_leaf_path(&list_first),
+ Some(vec!["items".to_string()])
+ );
Review Comment:
```suggestion
);
let list_before_scalar = DataType::Struct(
vec![
Arc::new(Field::new_list(
"items",
Field::new("item", DataType::Int32, true),
true,
)),
Arc::new(Field::new("tag", DataType::Utf8, true)),
]
.into(),
);
assert_eq!(
first_leaf_path(&list_before_scalar),
Some(vec!["tag".to_string()])
);
```
##########
datafusion/datasource-parquet/src/projection_read_plan.rs:
##########
@@ -1982,6 +2026,53 @@ mod test {
}
}
+ #[test]
+ fn first_leaf_path_descends_to_the_first_primitive() {
Review Comment:
```suggestion
fn first_leaf_path_prefers_scalar_leaves() {
```
--
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]
