Skip to content

feat: Implement native st_pointz, st_pointm, st_pointzm#13

Merged
jiayuasu merged 20 commits intoapache:mainfrom
petern48:st_pointzm
Sep 3, 2025
Merged

feat: Implement native st_pointz, st_pointm, st_pointzm#13
jiayuasu merged 20 commits intoapache:mainfrom
petern48:st_pointzm

Conversation

@petern48
Copy link
Contributor

@petern48 petern48 commented Sep 2, 2025

  • Implements native functions st_pointz, st_pointm, st_pointzm
  • Cleans up and makes that file more readable (previously a lot of magic numbers)
  • Add support for ArrayArrayArray and ArrayArrayArrayArray benchmark args

@petern48 petern48 requested a review from paleolimbot September 3, 2025 00:45
@petern48 petern48 marked this pull request as ready for review September 3, 2025 00:45
Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

We have some new helpers that I think will make the implementation a bit more concise that are worth a try...the test code looks great!

Comment on lines 288 to 305
let mut coords = Vec::with_capacity(num_coords);
let mut has_null = false;

for array in &coord_f64_arrays {
if array.is_null(i) {
has_null = true;
break;
} else {
coords.push(array.value(i));
}
}

if has_null {
builder.append_null();
} else {
populate_wkb_item(&mut item, &coords);
builder.append_value(&item);
}
Copy link
Member

Choose a reason for hiding this comment

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

The inside of this loop is a very performance-sensitive section and I think there are a few things we can do to help:

for ... {
  let values = (0..num_dimensions).iter().map(|j| coord_f64_arrays[j].value(i).collect::<Vec<_>>();
  let values_non_null = values.iter().take(num_dimensions).flatten().collect::<Vec<_>>();
  if values_non_null.len() == num_dimensions {
    write_wkb_point_header(&mut builder, dimensions)?;
    // I forget exactly how to write the f64s as little endian doubles to the builder but it's
    // the same as in write_wkb_xxxx()
   builder.append_value([]);
  } else {
    builder.append_null();
 }
}

This was the first ever function I wrote in sedonadb and we now have write_wkb_point_xx(). The builder implements Write, so you can write directly into it (pass it directly to write_wkb_xxx() functions).

Make sure you run cd rust/sedona-functions + cargo bench -- st_point to make sure your implementation doesn't slow down the XY case (the most common). It is probably worth separating the implementation completely for Z, M, and ZM (where performance is not as critical).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you were right. It was a notable perf regression (at least 100%), so I reverted st_point.rs back to the original version and moved everything else over into a new `st_pointzm.rs

@petern48 petern48 requested a review from paleolimbot September 3, 2025 19:24
Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

Thank you!

@jiayuasu jiayuasu merged commit 71742fb into apache:main Sep 3, 2025
5 checks passed
@petern48 petern48 deleted the st_pointzm branch September 3, 2025 21:25
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.

3 participants