Copilot commented on code in PR #1041:
URL: https://github.com/apache/sedona-db/pull/1041#discussion_r3552700903
##########
rust/sedona-spatial-join/src/operand_evaluator.rs:
##########
@@ -183,22 +185,30 @@ impl EvaluatedGeometryArray {
let mut wkbs = Vec::with_capacity(num_rows);
let mut x = Interval::empty();
let mut y = Interval::empty();
- geometry_array.iter_as_wkb(sedona_type, num_rows, |wkb_opt| {
- if let Some(wkb) = &wkb_opt {
+ geometry_array.iter_as_wkb_bytes(sedona_type, num_rows, |wkb_opt| {
+ if let Some(wkb_bytes) = &wkb_opt {
+ let wkb = wkb::reader::read_wkb(wkb_bytes)
+ .map_err(|e| exec_datafusion_err!("WKB parse failed:
{e}"))?;
x = Interval::empty();
y = Interval::empty();
- geo_traits_update_xy_bounds(wkb, &mut x, &mut y)
+ geo_traits_update_xy_bounds(&wkb, &mut x, &mut y)
.map_err(|e| sedona_internal_datafusion_err!("{e}"))?;
rect_vec.push(Bounds2D::new(x, y));
+
+ // Safety: The wkbs must reference buffers inside the
`geometry_array`. Since the `geometry_array` and
+ // `wkbs` are both owned by the `EvaluatedGeometryArray`, so
they have the same lifetime. We'll never
+ // have a situation where the `EvaluatedGeometryArray` is
dropped while the `wkbs` are still in use
+ // (guaranteed by the scope of the `wkbs` field and lifetime
signature of the `wkbs` method).
+ wkbs.push(unsafe {
+ transmute::<Option<wkb::reader::Wkb<'_>>,
Option<wkb::reader::Wkb<'_>>>(Some(
+ wkb,
+ ))
+ });
Review Comment:
The `unsafe transmute::<Option<Wkb<'_>>, Option<Wkb<'_>>>` here obscures
that the code is extending the WKB lifetime to match the
`Vec<Option<Wkb<'static>>>` field. Making the source/target lifetimes explicit
improves reviewability and reduces the risk of accidental “no-op” transmutes in
future edits (especially since CI treats clippy warnings as errors).
##########
rust/sedona-spatial-join/src/operand_evaluator.rs:
##########
@@ -223,8 +233,18 @@ impl EvaluatedGeometryArray {
// (guaranteed by the scope of the `wkbs` field and lifetime signature
of the `wkbs` method).
let num_rows = geometry_array.len();
let mut wkbs = Vec::with_capacity(num_rows);
- geometry_array.iter_as_wkb(sedona_type, num_rows, |wkb_opt| {
- wkbs.push(wkb_opt.map(|wkb| unsafe { transmute(wkb) }));
+ geometry_array.iter_as_wkb_bytes(sedona_type, num_rows, |wkb_opt| {
+ if let Some(wkb_bytes) = wkb_opt {
+ let wkb = wkb::reader::read_wkb(wkb_bytes)
+ .map_err(|e| exec_datafusion_err!("WKB parse failed:
{e}"))?;
+ wkbs.push(unsafe {
+ transmute::<Option<wkb::reader::Wkb<'_>>,
Option<wkb::reader::Wkb<'_>>>(Some(
+ wkb,
+ ))
+ });
Review Comment:
Same as above: this transmute is performing a lifetime extension into the
`Wkb<'static>` storage, but the current type parameters hide that intent.
Prefer an explicit `Wkb<'_> -> Wkb<'static>` transmute (or a small helper) so
the safety boundary is clear.
--
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]
