Skip to content

Commit

Permalink
Work around upstream compiler bug (#956)
Browse files Browse the repository at this point in the history
Closes #716. Ref
georust/geo#1255 (comment).

Note that this is the same underlying implementation as upstream geo in
<georust/geo#1255>. However, the trait-based
implementation hits this compiler regression
<rust-lang/rust#128887>,
<rust-lang/rust#131960>, 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.
  • Loading branch information
kylebarron authored Dec 20, 2024
1 parent d61e866 commit b4bc4f0
Show file tree
Hide file tree
Showing 12 changed files with 79 additions and 59 deletions.
8 changes: 3 additions & 5 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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/[email protected]
# with:
Expand All @@ -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
13 changes: 0 additions & 13 deletions .github/workflows/python-core-wheels.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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 }}
Expand Down
13 changes: 0 additions & 13 deletions .github/workflows/python-io-wheels.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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/[email protected]
Expand Down
8 changes: 2 additions & 6 deletions python/DEVELOP.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down
6 changes: 1 addition & 5 deletions python/geoarrow-core/DEVELOP.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 /
Expand Down
6 changes: 3 additions & 3 deletions rust/geoarrow/src/algorithm/geo/contains.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand Down Expand Up @@ -135,7 +135,7 @@ pub trait ContainsGeometry<Rhs> {

impl<G: GeometryTrait<T = f64>> ContainsGeometry<G> 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()
}
Expand All @@ -145,7 +145,7 @@ macro_rules! impl_contains_point {
($array:ty) => {
impl<G: GeometryTrait<T = f64>> ContainsGeometry<G> 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))
})
Expand Down
17 changes: 9 additions & 8 deletions rust/geoarrow/src/algorithm/geo/intersects.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -592,7 +593,7 @@ impl<G: GeometryTrait<T = f64>> IntersectsGeometry<G> 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)
})
Expand All @@ -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)
})
Expand All @@ -626,7 +627,7 @@ impl<G: GeometryTrait<T = f64>> IntersectsGeometry<G> for IndexedChunkedPointArr
type Output = ChunkedArray<BooleanArray>;

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()
Expand All @@ -639,7 +640,7 @@ macro_rules! impl_intersects {
type Output = ChunkedArray<BooleanArray>;

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()
Expand All @@ -666,7 +667,7 @@ impl<G: GeometryCollectionTrait<T = f64>> IntersectsGeometryCollection<G> 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)
})
Expand All @@ -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)
})
Expand All @@ -702,7 +703,7 @@ impl<G: GeometryCollectionTrait<T = f64>> IntersectsGeometryCollection<G>
type Output = ChunkedArray<BooleanArray>;

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()
Expand All @@ -715,7 +716,7 @@ macro_rules! impl_intersects {
type Output = ChunkedArray<BooleanArray>;

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()
Expand Down
54 changes: 54 additions & 0 deletions rust/geoarrow/src/io/geo.rs
Original file line number Diff line number Diff line change
@@ -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
//! <https://github.com/georust/geo/pull/1255>. However, the trait-based implementation hits this
//! compiler regression <https://github.com/rust-lang/rust/issues/128887>,
//! <https://github.com/rust-lang/rust/issues/131960>, 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<T: CoordNum>(geometry: &impl GeometryTrait<T = T>) -> Geometry<T> {
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<T: CoordNum>(
geometry_collection: &impl GeometryCollectionTrait<T = T>,
) -> GeometryCollection<T> {
GeometryCollection::new_from(
geometry_collection
.geometries()
.map(|geometry| geometry_to_geo(&geometry))
.collect(),
)
}
1 change: 1 addition & 0 deletions rust/geoarrow/src/io/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down
4 changes: 2 additions & 2 deletions rust/geoarrow/src/scalar/binary/scalar.rs
Original file line number Diff line number Diff line change
@@ -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};

Expand Down Expand Up @@ -77,7 +77,7 @@ impl<O: OffsetSizeTrait> AsRef<[u8]> for WKB<'_, O> {

impl<O: OffsetSizeTrait> From<&WKB<'_, O>> for geo::Geometry {
fn from(value: &WKB<'_, O>) -> Self {
value.parse().unwrap().to_geometry()
geometry_to_geo(&value.parse().unwrap())
}
}

Expand Down
4 changes: 2 additions & 2 deletions rust/geoarrow/src/scalar/geometry/scalar.rs
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -241,7 +241,7 @@ impl From<Geometry<'_>> for geo::Geometry {

impl From<&Geometry<'_>> for geo::Geometry {
fn from(value: &Geometry<'_>) -> Self {
ToGeoGeometry::to_geometry(value)
geometry_to_geo(value)
}
}

Expand Down
4 changes: 2 additions & 2 deletions rust/geoarrow/src/scalar/geometrycollection/scalar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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)
}
}

Expand Down

0 comments on commit b4bc4f0

Please sign in to comment.