Skip to content

Conversation

@zhangfengcdt
Copy link

This PR adds pluggable distance metrics for RTree neighbor searches, enabling Euclidean, Haversine, and Spheroid distance calculations. This allows users to choose appropriate distance calculations for different coordinate systems (planar vs geographic).

The distance metric system is designed to be extensible, allowing users to implement custom metrics (e.g., Manhattan distance, custom caching strategies, or WKB decoding).

  Add pluggable distance metrics for RTree neighbor searches, enabling
  Euclidean, Haversine, and Spheroid distance calculations. This allows
  users to choose appropriate distance calculations for different coordinate
  systems (planar vs geographic).
Copy link
Owner

@kylebarron kylebarron left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

Here are some initial comments. I need to think more about the changes in trait.rs

/// Calculate the distance between two geometries.
/// This method is used by geometry-based neighbor searches to compute the actual
/// distance between a query geometry and an item geometry.
fn distance_to_geometry(&self, geom1: &Geometry<f64>, geom2: &Geometry<f64>) -> N;
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm I'd prefer for these to take in &impl GeometryTrait<T = f64>, but I suppose that's a non-breaking change in the future.

Kontinuation added a commit to apache/sedona-db that referenced this pull request Oct 10, 2025
…rkspace, eliminating geo, wkt and wkb forks (#203)

This is the final step of the forked dependency elimination plan: https://github.com/apache/sedona-db/pull/165/files. sedona-geo-generic-alg will go live and replace the original wherobots/geo dependency. We also replace forked wkb and wkt dependencies with the latest release versions.

This patch depends on wherobots/geo-index#7 to resolve a geo version conflict.

Remaining tasks:

* geo-index will be the only forked dependency, and we are [submitting patches](kylebarron/geo-index#141) to put the nearest neighbour search APIs needed by sedona-db to the upstream.
* We need a released version of [georust/wkb](https://github.com/georust/wkb) before releasing 0.2.0.
- Feature Flag Implementation (geo-types pattern)
- Removed top-level re-exports from rtree/mod.rs
- Removed redundant geo-types dependency
- Removed duplicate axis_dist function (now in trait.rs)
- Made entire distance module conditional on use-geo_0_31 feature
- Trait architecture change: created SimpleDistanceMetric (no geo dependency)
- Tests run successfully with and without use-geo_0_31 feature flag
@zhangfengcdt
Copy link
Author

@kylebarron Thanks for the thorough review! I've addressed all the feedback:

  1. Feature flag pattern: Updated to follow the geo-types pattern exactly:

    • Feature flag: use-geo_0_31 = ["geo_0_31"]
    • Dependency: geo_0_31 = { package = "geo", version = "0.31" }
    • This allows future geo versions without breaking changes to geo-index
  2. Removed redundant geo-types dependency: already provided by geo's re-exports

  3. Removed top-level re-exports: users now access distance functionality via rtree::distance::

  4. Removed duplicate axis_dist: now uses the public axis_dist from trait.rs

  5. Feature-gated geo functionality: The entire distance module is now conditional on use-geo_0_31

  6. Trait architecture:

    • Created SimpleDistanceMetric (no geo dependency)
    • DistanceMetric extends SimpleDistanceMetric and adds geometry support
    • This allows custom distance metrics without requiring the geo dependency
  7. GeometryTrait TODO: Added a TODO comment noting the future enhancement to use &impl GeometryTrait<T = f64> instead of concrete Geometry type

All tests pass both with and without the feature flag enabled.

Thanks for the review again and please let me know any further changes you'd like to have on this PR.

@jiayuasu
Copy link

@kylebarron Hi Kyle, can you take a look of this PR?

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