You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2026/08/04 14:43:46 UTC
[PR] fix(rust/sedona-geoparquet): Handle intermediary columns in projection expressions in GeoParquet reader [sedona-db]
paleolimbot opened a new pull request, #1116:
URL: https://github.com/apache/sedona-db/pull/1116
This PR fixes a workaround we have for metadata getting dropped in the Parquet implementation's try_pushdown_projection. Because we advertize the file schema with different field metadata than the inner opener, we need to wrap the expressions (or UDF calls fail because there is no type metadata). I had thought the projection expressions we see had already been normalized to the file schema but this doesn't seem to be the case (i.e., we see Column references for other things, too).
Closes #1115.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(rust/sedona-geoparquet): Handle intermediary columns in projection expressions in GeoParquet reader [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #1116:
URL: https://github.com/apache/sedona-db/pull/1116#discussion_r3738735140
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -391,7 +391,34 @@ impl FileFormat for GeoParquetFormat {
)
.unwrap();
source.options = self.options.clone();
- Arc::new(source)
+
+ // DataFusion 52 has an issue where field metadata (like ARROW:extension:name)
+ // is stripped when evaluating embedded projections in ParquetOpener. This is
+ // because the batch schema comes from the parquet reader (which doesn't have
+ // extension metadata), and Column::return_field() looks up fields from that schema.
+ // This isn't a bug in DataFusion because we're the ones that advertised the table
+ // schema as having metadata'd expressions in the first place.
+ //
+ // We fix this by wrapping Column expressions with MetadataPreservingColumn,
+ // which stores the correct field from the file schema and returns it from
Review Comment:
There's a difference between the table schema that we have access to and the table schema the Parquet opener uses to evaluate projections / filters. This is not my favourite workaround...the idea is to "just wrap" the Parquet DataFusion implementation but it's accumulated quite a bit of workarounds and it's not as clean as I would like it to be.
All of this was rewritten on the Parquet end for each of the 53, 54, and 55 releases so we will have to redo these workarounds a few times in our future š®āšØ
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(rust/sedona-geoparquet): Handle intermediary columns in projection expressions in GeoParquet reader [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #1116:
URL: https://github.com/apache/sedona-db/pull/1116
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(rust/sedona-geoparquet): Handle intermediary columns in projection expressions in GeoParquet reader [sedona-db]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1116:
URL: https://github.com/apache/sedona-db/pull/1116#discussion_r3729377305
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -692,7 +703,7 @@ fn wrap_expr_columns(
if let Some(column) = node.as_any().downcast_ref::<Column>() {
let index = column.index();
let field = file_schema.field(index);
- // Only wrap columns that have extension metadata to preserve
+ // Only wrap columns that have extension metadata
if field.metadata().contains_key("ARROW:extension:name") {
Review Comment:
`file_schema.field(index)` will panic if `index` is out of bounds. Given this code exists specifically to harden projection handling, it should avoid panicking and instead skip wrapping (or return an error) when the column index doesn't exist in the file schema.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(rust/sedona-geoparquet): Handle intermediary columns in projection expressions in GeoParquet reader [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #1116:
URL: https://github.com/apache/sedona-db/pull/1116#discussion_r3738737677
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -691,8 +702,15 @@ fn wrap_expr_columns(
expr.transform_down(|node| {
if let Some(column) = node.as_any().downcast_ref::<Column>() {
let index = column.index();
+
+ if index >= file_schema.fields().len() {
+ return sedona_internal_err!(
+ "Unexpected projection expression in GeoParquet source: index {index} out of bounds"
+ );
Review Comment:
I added the deferred error mechanism to make sure this gets caught
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -1081,4 +1099,96 @@ mod test {
let geo_source_with_predicate = geo_source.with_predicate(predicate);
assert!(geo_source_with_predicate.inner.filter().is_some());
}
+
+ /// Test that columns with extension metadata are correctly wrapped
+ #[test]
+ fn test_wrap_expr_columns_wraps_geometry_column() {
+ let mut metadata = HashMap::new();
+ metadata.insert(
+ "ARROW:extension:name".to_string(),
+ "geoarrow.wkb".to_string(),
+ );
+ let file_schema = Schema::new(vec![
+ Field::new("geometry", DataType::Binary, true).with_metadata(metadata)
+ ]);
+
+ // Column expression for the geometry column (index 0 in file schema)
+ let geometry_column: Arc<dyn PhysicalExpr> = Arc::new(Column::new("geometry", 0));
+
+ let result = wrap_expr_columns(geometry_column, &file_schema).unwrap();
+
+ // The result should be wrapped in MetadataPreservingColumn
+ assert!(result
+ .as_any()
+ .downcast_ref::<MetadataPreservingColumn>()
+ .is_some());
+ }
+
+ /// Test that columns without extension metadata are not wrapped
+ #[test]
+ fn test_wrap_expr_columns_skips_non_geometry_column() {
+ let mut metadata = HashMap::new();
+ metadata.insert(
+ "ARROW:extension:name".to_string(),
+ "geoarrow.wkb".to_string(),
+ );
+ let file_schema = Schema::new(vec![
+ Field::new("name", DataType::Utf8, true),
+ Field::new("geometry", DataType::Binary, true).with_metadata(metadata),
+ ]);
+
+ // Column expression for a non-geometry column (no extension metadata)
+ let name_column: Arc<dyn PhysicalExpr> = Arc::new(Column::new("name", 0));
+
+ let result = wrap_expr_columns(name_column, &file_schema).unwrap();
+
+ // The result should NOT be wrapped (still a Column)
+ assert!(result.as_any().downcast_ref::<Column>().is_some());
+ }
+
+ /// Test that column index out of file schema bounds returns an error
+ #[test]
+ fn test_wrap_expr_columns_errors_on_out_of_bounds_index() {
+ let file_schema = Schema::new(vec![
+ Field::new("name", DataType::Utf8, true),
+ Field::new("age", DataType::Int32, true),
+ ]);
+
+ // Column expression with index 5, but schema only has 2 fields (indices 0 and 1)
+ let out_of_bounds_column: Arc<dyn PhysicalExpr> = Arc::new(Column::new("phantom", 5));
+
+ let result = wrap_expr_columns(out_of_bounds_column, &file_schema);
+
+ assert!(result.is_err());
+ let err_msg = result.unwrap_err().to_string();
+ assert!(
+ err_msg.contains("index 5 out of bounds"),
+ "Error message should mention out of bounds index, got: {err_msg}"
+ );
+ }
+
+ /// Integration test for projection with multiple derived columns plus geometry accessor
+ /// Regression test for https://github.com/apache/sedona-db/issues/1115
+ #[tokio::test]
+ async fn projection_with_derived_columns_and_geometry_accessor() {
+ let ctx = setup_context();
+ let example = test_geoparquet("example", "geometry").unwrap();
+
+ // Query that adds multiple literal columns alongside the scanned geometry
+ // This mimics: SELECT 'a' AS c1, 'b' AS c2, geometry FROM ...
+ let df = ctx
+ .sql(&format!(
+ "SELECT 'a' AS c1, 'b' AS c2, geometry FROM '{}' LIMIT 1",
+ example
+ ))
+ .await
+ .unwrap();
+
+ // This should not panic - the issue was "index out of bounds" when
+ // projection pushdown tried to wrap columns with indices beyond the
+ // file schema's bounds
+ let batches = df.collect().await.unwrap();
+ assert!(!batches.is_empty());
Review Comment:
I added this to the test!
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(rust/sedona-geoparquet): Handle intermediary columns in projection expressions in GeoParquet reader [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #1116:
URL: https://github.com/apache/sedona-db/pull/1116#discussion_r3738717845
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -391,7 +391,34 @@ impl FileFormat for GeoParquetFormat {
)
.unwrap();
source.options = self.options.clone();
- Arc::new(source)
+
+ // DataFusion 52 has an issue where field metadata (like ARROW:extension:name)
+ // is stripped when evaluating embedded projections in ParquetOpener. This is
+ // because the batch schema comes from the parquet reader (which doesn't have
+ // extension metadata), and Column::return_field() looks up fields from that schema.
+ // This isn't a bug in DataFusion because we're the ones that advertised the table
+ // schema as having metadata'd expressions in the first place.
+ //
+ // We fix this by wrapping Column expressions with MetadataPreservingColumn,
+ // which stores the correct field from the file schema and returns it from
+ // return_field() regardless of the input schema.
+ let initial_projection = source.projection().cloned().unwrap_or_else(|| {
+ let indices: Vec<usize> =
+ (0..source.table_schema().table_schema().fields().len()).collect();
+ ProjectionExprs::from_indices(&indices, source.table_schema().table_schema())
+ });
+ let projection_with_types = wrap_columns_with_metadata_preserving(
+ initial_projection,
+ source.table_schema().table_schema(),
+ )
+ .map(|projection_with_types| source.try_pushdown_projection(&projection_with_types));
+
+ // These are both failable but we can't fail here, so fall back to the original
+ // source.
+ match projection_with_types {
+ Ok(Ok(Some(modified))) => modified,
+ _ => Arc::new(source),
Review Comment:
That's a great point...it's not expected to fail but I added a deferred error mechanism to make sure we know if it does
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(rust/sedona-geoparquet): Handle intermediary columns in projection expressions in GeoParquet reader [sedona-db]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #1116:
URL: https://github.com/apache/sedona-db/pull/1116#discussion_r3738271197
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -391,7 +391,34 @@ impl FileFormat for GeoParquetFormat {
)
.unwrap();
source.options = self.options.clone();
- Arc::new(source)
+
+ // DataFusion 52 has an issue where field metadata (like ARROW:extension:name)
+ // is stripped when evaluating embedded projections in ParquetOpener. This is
+ // because the batch schema comes from the parquet reader (which doesn't have
+ // extension metadata), and Column::return_field() looks up fields from that schema.
+ // This isn't a bug in DataFusion because we're the ones that advertised the table
+ // schema as having metadata'd expressions in the first place.
+ //
+ // We fix this by wrapping Column expressions with MetadataPreservingColumn,
+ // which stores the correct field from the file schema and returns it from
+ // return_field() regardless of the input schema.
+ let initial_projection = source.projection().cloned().unwrap_or_else(|| {
+ let indices: Vec<usize> =
+ (0..source.table_schema().table_schema().fields().len()).collect();
+ ProjectionExprs::from_indices(&indices, source.table_schema().table_schema())
+ });
+ let projection_with_types = wrap_columns_with_metadata_preserving(
+ initial_projection,
+ source.table_schema().table_schema(),
+ )
+ .map(|projection_with_types| source.try_pushdown_projection(&projection_with_types));
+
+ // These are both failable but we can't fail here, so fall back to the original
+ // source.
+ match projection_with_types {
+ Ok(Ok(Some(modified))) => modified,
+ _ => Arc::new(source),
Review Comment:
Is it ok to silently not wrap? When do we expect this case to hit?
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -691,8 +702,15 @@ fn wrap_expr_columns(
expr.transform_down(|node| {
if let Some(column) = node.as_any().downcast_ref::<Column>() {
let index = column.index();
+
+ if index >= file_schema.fields().len() {
+ return sedona_internal_err!(
+ "Unexpected projection expression in GeoParquet source: index {index} out of bounds"
+ );
Review Comment:
This would get swallowed by the match at line 418.
Really wouldn't expect this to even happen since schema and expr come from the same place
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -391,7 +391,34 @@ impl FileFormat for GeoParquetFormat {
)
.unwrap();
source.options = self.options.clone();
- Arc::new(source)
+
+ // DataFusion 52 has an issue where field metadata (like ARROW:extension:name)
+ // is stripped when evaluating embedded projections in ParquetOpener. This is
+ // because the batch schema comes from the parquet reader (which doesn't have
+ // extension metadata), and Column::return_field() looks up fields from that schema.
+ // This isn't a bug in DataFusion because we're the ones that advertised the table
+ // schema as having metadata'd expressions in the first place.
+ //
+ // We fix this by wrapping Column expressions with MetadataPreservingColumn,
+ // which stores the correct field from the file schema and returns it from
Review Comment:
field from table schema right? thats kind of the idea here that the table might have type metadata that isnt correctly reflected in the file?
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -1081,4 +1099,96 @@ mod test {
let geo_source_with_predicate = geo_source.with_predicate(predicate);
assert!(geo_source_with_predicate.inner.filter().is_some());
}
+
+ /// Test that columns with extension metadata are correctly wrapped
+ #[test]
+ fn test_wrap_expr_columns_wraps_geometry_column() {
+ let mut metadata = HashMap::new();
+ metadata.insert(
+ "ARROW:extension:name".to_string(),
+ "geoarrow.wkb".to_string(),
+ );
+ let file_schema = Schema::new(vec![
+ Field::new("geometry", DataType::Binary, true).with_metadata(metadata)
+ ]);
+
+ // Column expression for the geometry column (index 0 in file schema)
+ let geometry_column: Arc<dyn PhysicalExpr> = Arc::new(Column::new("geometry", 0));
+
+ let result = wrap_expr_columns(geometry_column, &file_schema).unwrap();
+
+ // The result should be wrapped in MetadataPreservingColumn
+ assert!(result
+ .as_any()
+ .downcast_ref::<MetadataPreservingColumn>()
+ .is_some());
+ }
+
+ /// Test that columns without extension metadata are not wrapped
+ #[test]
+ fn test_wrap_expr_columns_skips_non_geometry_column() {
+ let mut metadata = HashMap::new();
+ metadata.insert(
+ "ARROW:extension:name".to_string(),
+ "geoarrow.wkb".to_string(),
+ );
+ let file_schema = Schema::new(vec![
+ Field::new("name", DataType::Utf8, true),
+ Field::new("geometry", DataType::Binary, true).with_metadata(metadata),
+ ]);
+
+ // Column expression for a non-geometry column (no extension metadata)
+ let name_column: Arc<dyn PhysicalExpr> = Arc::new(Column::new("name", 0));
+
+ let result = wrap_expr_columns(name_column, &file_schema).unwrap();
+
+ // The result should NOT be wrapped (still a Column)
+ assert!(result.as_any().downcast_ref::<Column>().is_some());
+ }
+
+ /// Test that column index out of file schema bounds returns an error
+ #[test]
+ fn test_wrap_expr_columns_errors_on_out_of_bounds_index() {
+ let file_schema = Schema::new(vec![
+ Field::new("name", DataType::Utf8, true),
+ Field::new("age", DataType::Int32, true),
+ ]);
+
+ // Column expression with index 5, but schema only has 2 fields (indices 0 and 1)
+ let out_of_bounds_column: Arc<dyn PhysicalExpr> = Arc::new(Column::new("phantom", 5));
+
+ let result = wrap_expr_columns(out_of_bounds_column, &file_schema);
+
+ assert!(result.is_err());
+ let err_msg = result.unwrap_err().to_string();
+ assert!(
+ err_msg.contains("index 5 out of bounds"),
+ "Error message should mention out of bounds index, got: {err_msg}"
+ );
+ }
+
+ /// Integration test for projection with multiple derived columns plus geometry accessor
+ /// Regression test for https://github.com/apache/sedona-db/issues/1115
+ #[tokio::test]
+ async fn projection_with_derived_columns_and_geometry_accessor() {
+ let ctx = setup_context();
+ let example = test_geoparquet("example", "geometry").unwrap();
+
+ // Query that adds multiple literal columns alongside the scanned geometry
+ // This mimics: SELECT 'a' AS c1, 'b' AS c2, geometry FROM ...
+ let df = ctx
+ .sql(&format!(
+ "SELECT 'a' AS c1, 'b' AS c2, geometry FROM '{}' LIMIT 1",
+ example
+ ))
+ .await
+ .unwrap();
+
+ // This should not panic - the issue was "index out of bounds" when
+ // projection pushdown tried to wrap columns with indices beyond the
+ // file schema's bounds
+ let batches = df.collect().await.unwrap();
+ assert!(!batches.is_empty());
Review Comment:
should we assert the metadata survives as well?
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -391,7 +391,34 @@ impl FileFormat for GeoParquetFormat {
)
.unwrap();
source.options = self.options.clone();
- Arc::new(source)
+
+ // DataFusion 52 has an issue where field metadata (like ARROW:extension:name)
+ // is stripped when evaluating embedded projections in ParquetOpener. This is
+ // because the batch schema comes from the parquet reader (which doesn't have
+ // extension metadata), and Column::return_field() looks up fields from that schema.
+ // This isn't a bug in DataFusion because we're the ones that advertised the table
+ // schema as having metadata'd expressions in the first place.
+ //
+ // We fix this by wrapping Column expressions with MetadataPreservingColumn,
+ // which stores the correct field from the file schema and returns it from
+ // return_field() regardless of the input schema.
+ let initial_projection = source.projection().cloned().unwrap_or_else(|| {
+ let indices: Vec<usize> =
+ (0..source.table_schema().table_schema().fields().len()).collect();
+ ProjectionExprs::from_indices(&indices, source.table_schema().table_schema())
Review Comment:
Isn't this just a copy of what is already in `ParquetSource.new`? When would we hit this case?
https://github.com/apache/datafusion/blob/branch-52/datafusion/datasource-parquet/src/source.rs#L308
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(rust/sedona-geoparquet): Handle intermediary columns in projection expressions in GeoParquet reader [sedona-db]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1116:
URL: https://github.com/apache/sedona-db/pull/1116#discussion_r3714676254
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -690,8 +690,14 @@ fn wrap_expr_columns(
) -> Result<Arc<dyn PhysicalExpr>> {
expr.transform_down(|node| {
if let Some(column) = node.as_any().downcast_ref::<Column>() {
- let index = column.index();
- let field = file_schema.field(index);
+ // Look up by column name instead of index, since the Column's index
+ // may refer to a different schema (e.g., a projected intermediate schema)
+ // rather than the file schema. If the column doesn't exist in the file
+ // schema, it's a derived column and doesn't need wrapping.
+ let Some((_, field)) = file_schema.column_with_name(column.name()) else {
+ return Ok(Transformed::no(node));
+ };
+
// Only wrap columns that have extension metadata to preserve
Review Comment:
This comment is incomplete (it ends mid-sentence). Please clarify what is being preserved (e.g., \"...to preserve Arrow extension type metadata\") so future readers understand why wrapping is conditional.
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -1081,4 +1087,110 @@ mod test {
let geo_source_with_predicate = geo_source.with_predicate(predicate);
assert!(geo_source_with_predicate.inner.filter().is_some());
}
+
+ /// Regression test for https://github.com/apache/sedona-db/issues/1115
+ ///
+ /// When projections contain Column expressions that reference columns not in the
+ /// file schema (e.g., derived literal columns like `'a' AS c1`), the Column indices
+ /// refer to the projection's output schema, not the file schema. Looking up by index
+ /// would cause an "index out of bounds" panic when the index exceeds the file schema's
+ /// field count.
+ #[test]
+ fn test_wrap_expr_columns_with_derived_columns() {
+ // File schema has only one geometry column with extension metadata
+ let mut metadata = HashMap::new();
+ metadata.insert(
+ "ARROW:extension:name".to_string(),
+ "geoarrow.wkb".to_string(),
+ );
+ let file_schema = Schema::new(vec![
+ Field::new("geometry", DataType::Binary, true).with_metadata(metadata)
+ ]);
+
+ // Create a Column expression that references a column NOT in the file schema
+ // This simulates a derived column like `'a' AS c1` which would have index 0
+ // in the projection output but doesn't exist in the file schema
+ let derived_column: Arc<dyn PhysicalExpr> = Arc::new(Column::new("c1", 0));
Review Comment:
This regression test is described as preventing an \"index out of bounds\" panic when the `Column` index exceeds the file schemaās field count, but it constructs `Column::new(\"c1\", 0)` while `file_schema` has 1 fieldāso the old index-based implementation would not have panicked here (index 0 is in-bounds). To actually exercise the panic path and ensure the regression is covered, set the derived column index to something >= `file_schema.fields().len()` (e.g., `Column::new(\"c1\", 2)`) and update the assertions accordingly.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(rust/sedona-geoparquet): Handle intermediary columns in projection expressions in GeoParquet reader [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #1116:
URL: https://github.com/apache/sedona-db/pull/1116#discussion_r3728835701
##########
python/sedonadb-geopandas/tests/__init__.py:
##########
Review Comment:
This is a drive by fix...pytest objects to this file and won't collect this test in VSCode if it exists
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(rust/sedona-geoparquet): Handle intermediary columns in projection expressions in GeoParquet reader [sedona-db]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1116:
URL: https://github.com/apache/sedona-db/pull/1116#discussion_r3728786024
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -391,7 +391,32 @@ impl FileFormat for GeoParquetFormat {
)
.unwrap();
source.options = self.options.clone();
- Arc::new(source)
+
+ // DataFusion 52 has an issue where field metadata (like ARROW:extension:name)
+ // is stripped when evaluating embedded projections in ParquetOpener. This is
+ // because the batch schema comes from the parquet reader (which doesn't have
+ // extension metadata), and Column::return_field() looks up fields from that schema.
+ // This isn't a bug in DataFusion because we're the ones that advertised the table
+ // schema as having metadata'd expressions in the first place.
+ //
+ // We fix this by wrapping Column expressions with MetadataPreservingColumn,
+ // which stores the correct field from the file schema and returns it from
+ // return_field() regardless of the input schema.
+ let initial_projection = source.projection().cloned().unwrap_or_else(|| {
+ ProjectionExprs::from_indices(&[0], source.table_schema().table_schema())
+ });
Review Comment:
When `source.projection()` is `None`, this constructs a projection for only column 0 and pushes it down. That can silently change scan semantics for queries that otherwise read all columns (e.g. `SELECT *`) by dropping the remaining 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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(rust/sedona-geoparquet): Handle intermediary columns in projection expressions in GeoParquet reader [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #1116:
URL: https://github.com/apache/sedona-db/pull/1116#discussion_r3738722622
##########
rust/sedona-geoparquet/src/format.rs:
##########
@@ -391,7 +391,34 @@ impl FileFormat for GeoParquetFormat {
)
.unwrap();
source.options = self.options.clone();
- Arc::new(source)
+
+ // DataFusion 52 has an issue where field metadata (like ARROW:extension:name)
+ // is stripped when evaluating embedded projections in ParquetOpener. This is
+ // because the batch schema comes from the parquet reader (which doesn't have
+ // extension metadata), and Column::return_field() looks up fields from that schema.
+ // This isn't a bug in DataFusion because we're the ones that advertised the table
+ // schema as having metadata'd expressions in the first place.
+ //
+ // We fix this by wrapping Column expressions with MetadataPreservingColumn,
+ // which stores the correct field from the file schema and returns it from
+ // return_field() regardless of the input schema.
+ let initial_projection = source.projection().cloned().unwrap_or_else(|| {
+ let indices: Vec<usize> =
+ (0..source.table_schema().table_schema().fields().len()).collect();
+ ProjectionExprs::from_indices(&indices, source.table_schema().table_schema())
Review Comment:
We shouldn't hit this case, but if that gets removed on upgrade and set to None at least it's still correct.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org