From b4bc4f02fa1de96193029306a32ed720dde620d8 Mon Sep 17 00:00:00 2001 From: Kyle Barron Date: Fri, 20 Dec 2024 12:25:54 -0500 Subject: [PATCH] Work around upstream compiler bug (#956) Closes https://github.com/geoarrow/geoarrow-rs/issues/716. Ref https://github.com/georust/geo/pull/1255#issuecomment-2557320731. Note that this is the same underlying implementation as upstream geo in . However, the trait-based implementation hits this compiler regression , , which prevents from compiling in release mode on a stable Rust version. For some reason, the **function-based implementation** does not hit this regression, and thus allows building geoarrow without using latest nightly and a custom `RUSTFLAGS`. Note that it's only `GeometryTrait` and `GeometryCollectionTrait` that hit this compiler bug. Other traits can use the upstream impls. --- .github/workflows/ci.yml | 8 ++- .github/workflows/python-core-wheels.yml | 13 ----- .github/workflows/python-io-wheels.yml | 13 ----- python/DEVELOP.md | 8 +-- python/geoarrow-core/DEVELOP.md | 6 +-- rust/geoarrow/src/algorithm/geo/contains.rs | 6 +-- rust/geoarrow/src/algorithm/geo/intersects.rs | 17 +++--- rust/geoarrow/src/io/geo.rs | 54 +++++++++++++++++++ rust/geoarrow/src/io/mod.rs | 1 + rust/geoarrow/src/scalar/binary/scalar.rs | 4 +- rust/geoarrow/src/scalar/geometry/scalar.rs | 4 +- .../src/scalar/geometrycollection/scalar.rs | 4 +- 12 files changed, 79 insertions(+), 59 deletions(-) create mode 100644 rust/geoarrow/src/io/geo.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f822e4846..c00cdf13b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -103,9 +103,7 @@ jobs: # - uses: actions/checkout@v4 # with: # submodules: "recursive" - # # We use nightly for now so that we can pass RUSTFLAGS below to work around - # # https://github.com/geoarrow/geoarrow-rs/issues/716 - # - uses: dtolnay/rust-toolchain@nightly + # - uses: dtolnay/rust-toolchain@stable # - uses: Swatinem/rust-cache@v2 # - uses: prefix-dev/setup-pixi@v0.8.1 # with: @@ -118,6 +116,6 @@ jobs: # echo "PKG_CONFIG_PATH=$(pwd)/build/.pixi/envs/default/lib/pkgconfig" >> "$GITHUB_ENV" # echo "LD_LIBRARY_PATH=$(pwd)/build/.pixi/envs/default/lib" >> "$GITHUB_ENV" # - name: Build benchmarks with no features - # run: RUSTFLAGS="-Zinline-mir=no" cargo bench --no-run + # run: cargo bench --no-run # - name: Build benchmarks with all features - # run: RUSTFLAGS="-Zinline-mir=no" cargo bench --no-run --all-features + # run: cargo bench --no-run --all-features diff --git a/.github/workflows/python-core-wheels.yml b/.github/workflows/python-core-wheels.yml index af89858f3..b50cf353a 100644 --- a/.github/workflows/python-core-wheels.yml +++ b/.github/workflows/python-core-wheels.yml @@ -45,10 +45,7 @@ jobs: python-version: 3.x - name: Build wheels uses: PyO3/maturin-action@v1 - env: - RUSTFLAGS: "-Zinline-mir=no" with: - rust-toolchain: nightly target: ${{ matrix.platform.target }} args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 -i 3.13 --manifest-path python/${{ matrix.module }}/Cargo.toml sccache: "true" @@ -83,8 +80,6 @@ jobs: # python-version: 3.x # - name: Build wheels # uses: PyO3/maturin-action@v1 - # env: - # RUSTFLAGS: "-Zinline-mir=no" # with: # target: ${{ matrix.platform.target }} # args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 -i 3.13 --manifest-path python/${{ matrix.module }}/Cargo.toml @@ -116,10 +111,7 @@ jobs: architecture: ${{ matrix.platform.target }} - name: Build wheels uses: PyO3/maturin-action@v1 - env: - RUSTFLAGS: "-Zinline-mir=no" with: - rust-toolchain: nightly target: ${{ matrix.platform.target }} args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 --manifest-path python/${{ matrix.module }}/Cargo.toml sccache: "true" @@ -149,10 +141,7 @@ jobs: python-version: 3.x - name: Build wheels uses: PyO3/maturin-action@v1 - env: - RUSTFLAGS: "-Zinline-mir=no" with: - rust-toolchain: nightly target: ${{ matrix.platform.target }} args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 -i 3.13 --manifest-path python/${{ matrix.module }}/Cargo.toml sccache: "true" @@ -191,8 +180,6 @@ jobs: - run: pip install pyodide-build - name: Build wheels uses: PyO3/maturin-action@v1 - env: - RUSTFLAGS: "-Zinline-mir=no" with: rust-toolchain: nightly target: ${{ matrix.platform.target }} diff --git a/.github/workflows/python-io-wheels.yml b/.github/workflows/python-io-wheels.yml index 4a2ba9c9a..976d444e2 100644 --- a/.github/workflows/python-io-wheels.yml +++ b/.github/workflows/python-io-wheels.yml @@ -50,10 +50,7 @@ jobs: - name: Build wheels uses: PyO3/maturin-action@v1 - env: - RUSTFLAGS: "-Zinline-mir=no" with: - rust-toolchain: nightly target: ${{ matrix.platform.target }} # As of Nov 2024, it was necessary to manually specify -i 3.13 to get # maturin to find the executable. --find-interpreter did not find it. @@ -88,10 +85,7 @@ jobs: - name: Build wheels - ${{ matrix.platform.target }} uses: PyO3/maturin-action@v1 - env: - RUSTFLAGS: "-Zinline-mir=no" with: - rust-toolchain: nightly target: ${{ matrix.platform.target }} args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 -i 3.13 -m python/geoarrow-io/Cargo.toml sccache: "true" @@ -124,10 +118,7 @@ jobs: - name: Build wheels uses: PyO3/maturin-action@v1 - env: - RUSTFLAGS: "-Zinline-mir=no" with: - rust-toolchain: nightly target: ${{ matrix.target }} args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 -m python/geoarrow-io/Cargo.toml @@ -162,10 +153,8 @@ jobs: # - name: Build wheels # uses: PyO3/maturin-action@v1 # with: - # rust-toolchain: nightly # target: ${{ matrix.target }} # manylinux: musllinux_1_2 - # TODO: update rustflags env # args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 -i 3.13 -m python/geoarrow-io/Cargo.toml # - name: Install built wheel @@ -206,10 +195,8 @@ jobs: # - name: Build wheels # uses: PyO3/maturin-action@v1 # with: - # rust-toolchain: nightly # target: ${{ matrix.platform.target }} # manylinux: musllinux_1_2 - # TODO: update rustflags env # args: --release --out dist -i 3.9 -i 3.10 -i 3.11 -i 3.12 -i 3.13 -m python/geoarrow-io/Cargo.toml # - uses: uraimo/run-on-arch-action@v2.5.1 diff --git a/python/DEVELOP.md b/python/DEVELOP.md index 95f083991..e4c2f121f 100644 --- a/python/DEVELOP.md +++ b/python/DEVELOP.md @@ -83,21 +83,17 @@ PYODIDE_EMSCRIPTEN_VERSION=$(pyodide config get emscripten_version) source ~/github/emscripten-core/emsdk/emsdk_env.sh ``` -Note that the addition of `RUSTFLAGS="-Zinline-mir=no"` is temporary due to https://github.com/rust-lang/rust/issues/128887. - Build `geoarrow-rust-core` and `geoarrow-rust-io`: ```bash -RUSTFLAGS="-Zinline-mir=no" RUSTUP_TOOLCHAIN=nightly \ - maturin build \ +maturin build \ --release \ --no-default-features \ -o dist \ -m geoarrow-core/Cargo.toml \ --target wasm32-unknown-emscripten \ -i python3.11 -RUSTFLAGS="-Zinline-mir=no" RUSTUP_TOOLCHAIN=nightly \ - maturin build \ +maturin build \ --release \ --no-default-features \ -o dist \ diff --git a/python/geoarrow-core/DEVELOP.md b/python/geoarrow-core/DEVELOP.md index 9133677f3..8cf3fb2d5 100644 --- a/python/geoarrow-core/DEVELOP.md +++ b/python/geoarrow-core/DEVELOP.md @@ -27,14 +27,10 @@ source emsdk_env.sh cd .. ``` -- The `RUSTFLAGS` is temporary to get around this compiler bug. -- You must use rust nightly - You must use `--no-default-features` to remove any async support. `tokio` does not compile for emscripten. ```bash -RUSTFLAGS='-Zinline-mir=no' / - RUSTUP_TOOLCHAIN=nightly / - maturin build / +maturin build / --no-default-features / --release / -o dist / diff --git a/rust/geoarrow/src/algorithm/geo/contains.rs b/rust/geoarrow/src/algorithm/geo/contains.rs index 278dd8fec..814acc40d 100644 --- a/rust/geoarrow/src/algorithm/geo/contains.rs +++ b/rust/geoarrow/src/algorithm/geo/contains.rs @@ -2,11 +2,11 @@ use crate::algorithm::native::{Binary, Unary}; use crate::array::*; use crate::datatypes::NativeType; use crate::error::GeoArrowError; +use crate::io::geo::geometry_to_geo; use crate::trait_::NativeScalar; use crate::NativeArray; use arrow_array::BooleanArray; use geo::Contains as _Contains; -use geo_traits::to_geo::*; use geo_traits::GeometryTrait; /// Checks if `rhs` is completely contained within `self`. @@ -135,7 +135,7 @@ pub trait ContainsGeometry { impl> ContainsGeometry for PointArray { fn contains(&self, rhs: &G) -> BooleanArray { - let rhs = rhs.to_geometry(); + let rhs = geometry_to_geo(rhs); self.try_unary_boolean::<_, GeoArrowError>(|geom| Ok(geom.to_geo().contains(&rhs))) .unwrap() } @@ -145,7 +145,7 @@ macro_rules! impl_contains_point { ($array:ty) => { impl> ContainsGeometry for $array { fn contains(&self, rhs: &G) -> BooleanArray { - let rhs = rhs.to_geometry(); + let rhs = geometry_to_geo(rhs); self.try_unary_boolean::<_, GeoArrowError>(|geom| { Ok(geom.to_geo_geometry().contains(&rhs)) }) diff --git a/rust/geoarrow/src/algorithm/geo/intersects.rs b/rust/geoarrow/src/algorithm/geo/intersects.rs index fb0d1c396..6f5675d25 100644 --- a/rust/geoarrow/src/algorithm/geo/intersects.rs +++ b/rust/geoarrow/src/algorithm/geo/intersects.rs @@ -1,6 +1,7 @@ use crate::chunked_array::ChunkedArray; use crate::indexed::array::*; use crate::indexed::chunked::*; +use crate::io::geo::{geometry_collection_to_geo, geometry_to_geo}; use crate::trait_::NativeScalar; use arrow_array::BooleanArray; use geo::{BoundingRect, Intersects as _Intersects}; @@ -592,7 +593,7 @@ impl> IntersectsGeometry for IndexedPointArray { type Output = BooleanArray; fn intersects(&self, rhs: &G) -> Self::Output { - let rhs = rhs.to_geometry(); + let rhs = geometry_to_geo(rhs); self.unary_boolean(&rhs.bounding_rect().unwrap(), |geom| { geom.to_geo().intersects(&rhs) }) @@ -605,7 +606,7 @@ macro_rules! impl_intersects { type Output = BooleanArray; fn intersects(&self, rhs: &G) -> Self::Output { - let rhs = rhs.to_geometry(); + let rhs = geometry_to_geo(rhs); self.unary_boolean(&rhs.bounding_rect().unwrap(), |geom| { geom.to_geo().intersects(&rhs) }) @@ -626,7 +627,7 @@ impl> IntersectsGeometry for IndexedChunkedPointArr type Output = ChunkedArray; fn intersects(&self, rhs: &G) -> Self::Output { - let rhs = rhs.to_geometry(); + let rhs = geometry_to_geo(rhs); self.map(|chunk| IntersectsGeometry::intersects(chunk, &rhs)) .try_into() .unwrap() @@ -639,7 +640,7 @@ macro_rules! impl_intersects { type Output = ChunkedArray; fn intersects(&self, rhs: &G) -> Self::Output { - let rhs = rhs.to_geometry(); + let rhs = geometry_to_geo(rhs); self.map(|chunk| IntersectsGeometry::intersects(chunk, &rhs)) .try_into() .unwrap() @@ -666,7 +667,7 @@ impl> IntersectsGeometryCollection for In type Output = BooleanArray; fn intersects(&self, rhs: &G) -> Self::Output { - let rhs = rhs.to_geometry_collection(); + let rhs = geometry_collection_to_geo(rhs); self.unary_boolean(&rhs.bounding_rect().unwrap(), |geom| { geom.to_geo().intersects(&rhs) }) @@ -679,7 +680,7 @@ macro_rules! impl_intersects { type Output = BooleanArray; fn intersects(&self, rhs: &G) -> Self::Output { - let rhs = rhs.to_geometry_collection(); + let rhs = geometry_collection_to_geo(rhs); self.unary_boolean(&rhs.bounding_rect().unwrap(), |geom| { geom.to_geo().intersects(&rhs) }) @@ -702,7 +703,7 @@ impl> IntersectsGeometryCollection type Output = ChunkedArray; fn intersects(&self, rhs: &G) -> Self::Output { - let rhs = rhs.to_geometry_collection(); + let rhs = geometry_collection_to_geo(rhs); self.map(|chunk| IntersectsGeometryCollection::intersects(chunk, &rhs)) .try_into() .unwrap() @@ -715,7 +716,7 @@ macro_rules! impl_intersects { type Output = ChunkedArray; fn intersects(&self, rhs: &G) -> Self::Output { - let rhs = rhs.to_geometry_collection(); + let rhs = geometry_collection_to_geo(rhs); self.map(|chunk| IntersectsGeometryCollection::intersects(chunk, &rhs)) .try_into() .unwrap() diff --git a/rust/geoarrow/src/io/geo.rs b/rust/geoarrow/src/io/geo.rs new file mode 100644 index 000000000..488ff137b --- /dev/null +++ b/rust/geoarrow/src/io/geo.rs @@ -0,0 +1,54 @@ +//! Convert structs that implement geo-traits to [geo-types] objects. +//! +//! Note that this is the same underlying implementation as upstream [geo] in +//! . However, the trait-based implementation hits this +//! compiler regression , +//! , which prevents from compiling in release +//! mode on a stable Rust version. For some reason, the **function-based implementation** does not +//! hit this regression, and thus allows building geoarrow without using latest nightly and a +//! custom `RUSTFLAGS`. +//! +//! Note that it's only `GeometryTrait` and `GeometryCollectionTrait` that hit this compiler bug. +//! Other traits can use the upstream impls. + +use geo::{CoordNum, Geometry, GeometryCollection}; + +use geo_traits::to_geo::{ + ToGeoLine, ToGeoLineString, ToGeoMultiLineString, ToGeoMultiPoint, ToGeoMultiPolygon, + ToGeoPoint, ToGeoPolygon, ToGeoRect, ToGeoTriangle, +}; +use geo_traits::{GeometryCollectionTrait, GeometryTrait, GeometryType}; + +/// Convert any Geometry to a [`Geometry`]. +/// +/// Only the first two dimensions will be kept. +pub fn geometry_to_geo(geometry: &impl GeometryTrait) -> Geometry { + use GeometryType::*; + + match geometry.as_type() { + Point(geom) => Geometry::Point(geom.to_point()), + LineString(geom) => Geometry::LineString(geom.to_line_string()), + Polygon(geom) => Geometry::Polygon(geom.to_polygon()), + MultiPoint(geom) => Geometry::MultiPoint(geom.to_multi_point()), + MultiLineString(geom) => Geometry::MultiLineString(geom.to_multi_line_string()), + MultiPolygon(geom) => Geometry::MultiPolygon(geom.to_multi_polygon()), + GeometryCollection(geom) => Geometry::GeometryCollection(geometry_collection_to_geo(geom)), + Rect(geom) => Geometry::Rect(geom.to_rect()), + Line(geom) => Geometry::Line(geom.to_line()), + Triangle(geom) => Geometry::Triangle(geom.to_triangle()), + } +} + +/// Convert any GeometryCollection to a [`GeometryCollection`]. +/// +/// Only the first two dimensions will be kept. +pub fn geometry_collection_to_geo( + geometry_collection: &impl GeometryCollectionTrait, +) -> GeometryCollection { + GeometryCollection::new_from( + geometry_collection + .geometries() + .map(|geometry| geometry_to_geo(&geometry)) + .collect(), + ) +} diff --git a/rust/geoarrow/src/io/mod.rs b/rust/geoarrow/src/io/mod.rs index 38eeadd36..a3ee35575 100644 --- a/rust/geoarrow/src/io/mod.rs +++ b/rust/geoarrow/src/io/mod.rs @@ -11,6 +11,7 @@ pub(crate) mod display; pub mod flatgeobuf; #[cfg(feature = "gdal")] pub mod gdal; +pub(crate) mod geo; pub mod geojson; pub mod geojson_lines; #[cfg(feature = "geos")] diff --git a/rust/geoarrow/src/scalar/binary/scalar.rs b/rust/geoarrow/src/scalar/binary/scalar.rs index 26e3062cc..d5035717a 100644 --- a/rust/geoarrow/src/scalar/binary/scalar.rs +++ b/rust/geoarrow/src/scalar/binary/scalar.rs @@ -1,8 +1,8 @@ use crate::error::Result; +use crate::io::geo::geometry_to_geo; use crate::trait_::NativeScalar; use arrow_array::{GenericBinaryArray, OffsetSizeTrait}; use geo::BoundingRect; -use geo_traits::to_geo::ToGeoGeometry; use geo_traits::GeometryTrait; use rstar::{RTreeObject, AABB}; @@ -77,7 +77,7 @@ impl AsRef<[u8]> for WKB<'_, O> { impl From<&WKB<'_, O>> for geo::Geometry { fn from(value: &WKB<'_, O>) -> Self { - value.parse().unwrap().to_geometry() + geometry_to_geo(&value.parse().unwrap()) } } diff --git a/rust/geoarrow/src/scalar/geometry/scalar.rs b/rust/geoarrow/src/scalar/geometry/scalar.rs index 179e66471..f0bd18068 100644 --- a/rust/geoarrow/src/scalar/geometry/scalar.rs +++ b/rust/geoarrow/src/scalar/geometry/scalar.rs @@ -1,7 +1,7 @@ use crate::algorithm::native::eq::geometry_eq; +use crate::io::geo::geometry_to_geo; use crate::scalar::*; use crate::trait_::NativeScalar; -use geo_traits::to_geo::ToGeoGeometry; use geo_traits::{ GeometryCollectionTrait, GeometryTrait, GeometryType, LineStringTrait, MultiLineStringTrait, MultiPointTrait, MultiPolygonTrait, PointTrait, PolygonTrait, RectTrait, UnimplementedLine, @@ -241,7 +241,7 @@ impl From> for geo::Geometry { impl From<&Geometry<'_>> for geo::Geometry { fn from(value: &Geometry<'_>) -> Self { - ToGeoGeometry::to_geometry(value) + geometry_to_geo(value) } } diff --git a/rust/geoarrow/src/scalar/geometrycollection/scalar.rs b/rust/geoarrow/src/scalar/geometrycollection/scalar.rs index b1cf9a4bb..635b2800d 100644 --- a/rust/geoarrow/src/scalar/geometrycollection/scalar.rs +++ b/rust/geoarrow/src/scalar/geometrycollection/scalar.rs @@ -2,12 +2,12 @@ use crate::algorithm::native::eq::geometry_collection_eq; use crate::array::util::OffsetBufferUtils; use crate::array::MixedGeometryArray; use crate::datatypes::Dimension; +use crate::io::geo::geometry_collection_to_geo; use crate::scalar::Geometry; use crate::trait_::ArrayAccessor; use crate::trait_::NativeScalar; use crate::NativeArray; use arrow_buffer::OffsetBuffer; -use geo_traits::to_geo::ToGeoGeometryCollection; use geo_traits::GeometryCollectionTrait; use rstar::{RTreeObject, AABB}; @@ -111,7 +111,7 @@ impl<'a> GeometryCollectionTrait for &'a GeometryCollection<'a> { impl From<&GeometryCollection<'_>> for geo::GeometryCollection { fn from(value: &GeometryCollection<'_>) -> Self { - value.to_geometry_collection() + geometry_collection_to_geo(value) } }