feat: Add sedona-geo-generic-alg to sedona-db #195
Conversation
Kontinuation
force-pushed
the
split-pr5-add-geo-generic-alg
branch
2 times, most recently
from
7886ab5 to
c0e244b
Compare
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
I know you're still working on this but I took a look...generally good although I do think there's a few things we can do to acknowledge the source/license inline.
| use sedona_geo_traits_ext::*; | ||
|
|
||
| use crate::{CoordFloat, CoordNum}; | ||
| use core::borrow::Borrow; | ||
|
|
There was a problem hiding this comment.
Much of these algorithms are copied from geo. Module level documentation along the lines of
//! Generic Area algorithm
//!
//! Ported (and contains copied code) from `geo::algorithm::area`:
//! <https://github.com/georust/geo/blob/09eed85233f706d42fc27ac9f1494bd904a3b7a7/geo/src/algorithm/area.rs>.
(1) for licensing/IP clarity and (2) so that one could do a periodic review of the ported algorithms to see if any bugs were fixed upstream since the commit listed in the file.
We can/should also acknowledge the source and license of this copied code in LICENSE (this is not quite a vendor but it is very similar and our other vendored code is listed there).
There was a problem hiding this comment.
I have added license header to all source files in rust/sedona-geo-generic-alg/src, and also added a license statement to the LICENSE file. All source files originates from 5d667f of georust/geo. We'll port bugfixes and performance improvements since this commit in subsequent PRs.
Kontinuation
force-pushed
the
split-pr5-add-geo-generic-alg
branch
from
c0e244b to
503178f
Compare
…date LICENSE docs: add upstream geo source attribution to remaining generic algorithms docs: add upstream geo source attribution to remaining line_measures modules docs: add upstream geo source attribution to intersects submodules and algorithm mod docs: add upstream geo attribution to geometry, lib, and utils modules license
Kontinuation
force-pushed
the
split-pr5-add-geo-generic-alg
branch
from
503178f to
8383741
Compare
paleolimbot
left a comment
paleolimbot
left a comment
There was a problem hiding this comment.
Just a few optional nits on the Cargo.toml to take or leave...thank you!
| version = "0.2.0" | ||
| authors = ["Apache Sedona <[email protected]>"] | ||
| license = "Apache-2.0" | ||
| homepage = "https://github.com/apache/sedona-db" | ||
| repository = "https://github.com/apache/sedona-db" | ||
| description = "geo algorithms refactored to work with sedona-geo-traits-ext" | ||
| readme = "README.md" | ||
| edition = "2021" |
There was a problem hiding this comment.
These can probably use { workspace = true} to avoid keeping them up to date?
There was a problem hiding this comment.
No. Currently sedona-geo-generic-alg is in its standalone workspace, not in the workspace of sedona-db. Adding it to the workspace of sedona-db will result in the most terrible dependency conflict. We'll add it to sedona-db's workspace and update this to inherit from workspace in the final step.
| [features] | ||
| default = ["multithreading"] | ||
| use-serde = ["serde", "geo-types/serde"] | ||
| multithreading = ["i_overlay/allow_multithreading", "geo-types/multithreading"] |
There was a problem hiding this comment.
I'm not sure if it's worth it or not, but I was able to remove these (plus rstar and serde) without affecting the build.
There was a problem hiding this comment.
These features are not needed for now. I'll remove them.
Kontinuation
force-pushed
the
split-pr5-add-geo-generic-alg
branch
from
acbd701 to
5fa879f
Compare
2 participants
This is part of the forked dependency elimination plan: #165. This PR depends on #194.
This PR moves geo-generic-alg from wherobots/geo to sedona-db and renamed it to sedona-geo-generic-alg. Currently it is a standalone crate and can be compiled using
cd rust/sedona-geo-generic-alg && cargo build. We'll update the Cargo.toml files in the final step to make it live.The code moved to sedona-db only contains the algorithms actually used by other parts of the project. There's a backup containing all ported algorithms here: https://github.com/Kontinuation/sedona-db/tree/full-migrate-generic-alg.