You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "kylebarron (via GitHub)" <gi...@apache.org> on 2025/03/31 18:21:40 UTC
[PR] Parquet: POC for handling struct child via StatisticsConverter [arrow-rs]
kylebarron opened a new pull request, #7365:
URL: https://github.com/apache/arrow-rs/pull/7365
# Which issue does this PR close?
Closes https://github.com/apache/arrow-rs/issues/7364. Also related to https://github.com/apache/datafusion/issues/10609 and https://github.com/apache/datafusion/issues/8334.
# Rationale for this change
Support some way to handle struct-typed columns in `StatisticsConverter`.
# What changes are included in this PR?
I think there are two ways to handle this:
1. Support struct-type columns directly, where the user can pass the name of a struct column, and all children are automatically handled. So `row_group_mins` would return a struct-typed Arrow array.
2. Adjust `StatisticsConverter::try_new` to handle a nested column name. So the user would pass `a.b.c`, which designates a primitive type, and we reuse existing converters. So `row_group_mins` would return a **primitive-typed** Arrow array.
This PR prototypes approach 2.
In principle both approaches would be good to have, but approach 1 looked more complex, and approach 2 at least provides a valid workaround.
# Are there any user-facing changes?
Yes, a breaking change to the signature of `StatisticsConverter` to use `ColumnPath` instead of `&str` for the `column_name`. This isn't _technically_ required; we could assume that a `.` in the column name is a field delimiter, but this seems unnecessary when `ColumnPath` already exists as a type.
(Similarly, it's weird to me that [`ProjectionMask::columns`](https://docs.rs/parquet/latest/parquet/arrow/struct.ProjectionMask.html#method.columns) takes in `&str` and not `&ColumnPath` as input)
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: POC for handling struct child via StatisticsConverter [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #7365:
URL: https://github.com/apache/arrow-rs/pull/7365#discussion_r2021852968
##########
parquet/src/arrow/arrow_reader/statistics.rs:
##########
@@ -1290,32 +1290,69 @@ impl<'a> StatisticsConverter<'a> {
///
/// * If the column is not found in the arrow schema
pub fn try_new<'b>(
- column_name: &'b str,
+ column: &'b ColumnPath,
arrow_schema: &'a Schema,
parquet_schema: &'a SchemaDescriptor,
) -> Result<Self> {
+ let mut fields = arrow_schema.fields();
+
+ let mut arrow_field = None;
// ensure the requested column is in the arrow schema
- let Some((_idx, arrow_field)) = arrow_schema.column_with_name(column_name) else {
+ for part in column.parts() {
+ if let Some((_idx, inner_arrow_field)) = fields.find(part) {
+ match inner_arrow_field.data_type() {
+ DataType::Struct(inner_fields) => {
+ fields = inner_fields;
+ }
+ _ => {
+ arrow_field = Some(inner_arrow_field);
+ }
+ }
+ } else {
+ return Err(arrow_err!(format!(
+ "Column '{}' not found in schema for statistics conversion",
+ column
+ )));
+ };
+ }
+
+ let arrow_field = arrow_field.ok_or(arrow_err!(format!(
+ "Column '{}' not found in schema for statistics conversion",
+ column
+ )))?;
+
+ let mask =
+ ProjectionMask::columns(parquet_schema, std::iter::once(column.string().as_str()));
Review Comment:
And use `ProjectionMask::columns`, which handles searching for nested Parquet fields
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: POC for handling struct child via StatisticsConverter [arrow-rs]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #7365: Parquet: POC for handling struct child via StatisticsConverter
URL: https://github.com/apache/arrow-rs/pull/7365
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: POC for handling struct child via StatisticsConverter [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #7365:
URL: https://github.com/apache/arrow-rs/pull/7365#discussion_r2021571329
##########
parquet/tests/arrow_reader/statistics.rs:
##########
@@ -2683,3 +2695,70 @@ mod test {
Arc::new(array)
}
}
+
+// To be removed before merging but a real-world use case
+#[cfg(test)]
+mod test_geoparquet {
+ use std::sync::Arc;
+
+ use arrow::array::AsArray;
+ use arrow::datatypes::Float32Type;
+ use object_store::aws::AmazonS3Builder;
+ use parquet::arrow::arrow_reader::ArrowReaderMetadata;
+ use parquet::arrow::async_reader::ParquetObjectReader;
+ use parquet::schema::types::ColumnPath;
+
+ use super::*;
+
+ #[tokio::test]
+ async fn test_struct_geoparquet() {
+ let store = Arc::new(
+ AmazonS3Builder::new()
+ .with_bucket_name("overturemaps-us-west-2")
+ .with_skip_signature(true)
+ .with_region("us-west-2")
+ .build()
+ .unwrap(),
+ );
+ let path = "release/2025-02-19.0/theme=addresses/type=address/part-00010-e084a2d7-fea9-41e5-a56f-e638a3307547-c000.zstd.parquet";
Review Comment:
This is public data from the [Overture project](https://overturemaps.org/) conforming to the [GeoParquet specification](https://github.com/opengeospatial/geoparquet). That defines a struct column, canonically called `bbox`, which [contains the fields `xmin`, `ymin`, `xmax`, `ymax`](https://github.com/opengeospatial/geoparquet/blob/v1.1.0%2Bp1/format-specs/geoparquet.md#covering). This allows for row group filtering for spatially-sorted data by inferring the spatial bounds of each row group from the statistics.
In order to use the Arrow statistics converter, which is way simpler than handling the statistics manually, we would need _some way_ to access these struct columns.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: POC for handling struct child via StatisticsConverter [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #7365:
URL: https://github.com/apache/arrow-rs/pull/7365#discussion_r2021851051
##########
parquet/src/arrow/arrow_reader/statistics.rs:
##########
@@ -1290,32 +1290,69 @@ impl<'a> StatisticsConverter<'a> {
///
/// * If the column is not found in the arrow schema
pub fn try_new<'b>(
- column_name: &'b str,
+ column: &'b ColumnPath,
arrow_schema: &'a Schema,
parquet_schema: &'a SchemaDescriptor,
) -> Result<Self> {
+ let mut fields = arrow_schema.fields();
+
+ let mut arrow_field = None;
// ensure the requested column is in the arrow schema
- let Some((_idx, arrow_field)) = arrow_schema.column_with_name(column_name) else {
+ for part in column.parts() {
+ if let Some((_idx, inner_arrow_field)) = fields.find(part) {
+ match inner_arrow_field.data_type() {
+ DataType::Struct(inner_fields) => {
+ fields = inner_fields;
+ }
+ _ => {
+ arrow_field = Some(inner_arrow_field);
+ }
+ }
Review Comment:
The core change: recursively look into the arrow field's children if the field is a struct
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: POC for handling struct child via StatisticsConverter [arrow-rs]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7365:
URL: https://github.com/apache/arrow-rs/pull/7365#issuecomment-4805869356
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: POC for handling struct child via StatisticsConverter [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on PR #7365:
URL: https://github.com/apache/arrow-rs/pull/7365#issuecomment-4005454768
It would still be nice to handle this (https://github.com/apache/arrow-rs/issues/7364 is still open) but I don't have time to spend on this.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Parquet: POC for handling struct child via StatisticsConverter [arrow-rs]
Posted by "Rafferty97 (via GitHub)" <gi...@apache.org>.
Rafferty97 commented on PR #7365:
URL: https://github.com/apache/arrow-rs/pull/7365#issuecomment-4000916823
@kylebarron I've noticed this PR has been sitting around for a while. I tried reproducing the original bug report in #7364 but it looks like some of the APIs have changed in the interim. Is this still something that needs fixing, or should we potentially close this one?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org