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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
