Skip to content

Conversation

@osipovartem
Copy link
Contributor

@osipovartem osipovartem commented Mar 5, 2025

Copilot AI review requested due to automatic review settings March 5, 2025 22:42
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This pull request adds new UDF implementations for computing minimum and maximum X/Y coordinates for geometries, along with associated test updates and registration changes.

  • Added macros to generate extremum UDFs (st_xmin, st_ymin, st_xmax, st_ymax) using a common helper function.
  • Updated existing UDFs in point, srid, dim, and line string modules to use the new invoke_with_args API.
  • Updated data_types to include GEOMETRY_COLLECTION_TYPE and adjusted parsing accordingly.

Reviewed Changes

File Description
crates/runtime/src/datafusion/functions/geospatial/accessors/geometry.rs Added extremum UDFs and a helper (get_extremum) for min/max coordinate extraction.
crates/runtime/src/datafusion/functions/geospatial/accessors/point.rs Updated point UDFs to use a macro and invoke_with_args API.
crates/runtime/src/datafusion/functions/geospatial/accessors/mod.rs Registered new geometry UDFs in the context.
crates/runtime/src/datafusion/functions/geospatial/data_types.rs Added GEOMETRY_COLLECTION_TYPE and updated parse_to_native_array accordingly.
crates/runtime/src/datafusion/functions/geospatial/accessors/srid.rs, dim.rs, line_string.rs Updated to use ScalarFunctionArgs and invoke_with_args for consistency.

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

crates/runtime/src/datafusion/functions/geospatial/accessors/geometry.rs:125

  • The helper function get_extremum calls ColumnarValue::values_to_arrays(args) twice to retrieve the same array. Consider storing the result in a variable and reusing it to avoid redundant computations.
let arg = ColumnarValue::values_to_arrays(args)? .into_iter().next() ...

Copy link
Contributor

@DanCodedThis DanCodedThis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@osipovartem osipovartem merged commit d59876f into main Mar 6, 2025
4 of 5 checks passed
@osipovartem osipovartem deleted the issues/323_geom_min_max branch March 6, 2025 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants