paleolimbot commented on code in PR #749:
URL: https://github.com/apache/sedona-db/pull/749#discussion_r3047948736
##########
rust/sedona-raster/src/affine_transformation.rs:
##########
@@ -124,139 +132,123 @@ pub fn to_raster_coordinate(
world_y: f64,
) -> Result<(i64, i64), ArrowError> {
let (rx, ry) =
- AffineMatrix::from_metadata(raster.metadata()).inv_transform(world_x,
world_y)?;
+
AffineMatrix::from_transform(raster.transform()).inv_transform(world_x,
world_y)?;
Ok((rx as i64, ry as i64))
}
#[cfg(test)]
mod tests {
use super::*;
- use crate::traits::{MetadataRef, RasterMetadata};
use approx::assert_relative_eq;
use std::f64::consts::FRAC_1_SQRT_2;
use std::f64::consts::PI;
+ /// Minimal RasterRef implementation for testing affine transforms.
struct TestRaster {
- metadata: RasterMetadata,
+ transform: [f64; 6],
+ }
+
+ impl TestRaster {
+ fn new(
+ origin_x: f64,
+ origin_y: f64,
+ scale_x: f64,
+ scale_y: f64,
+ skew_x: f64,
+ skew_y: f64,
+ ) -> Self {
+ Self {
+ transform: [origin_x, scale_x, skew_x, origin_y, skew_y,
scale_y],
+ }
+ }
}
impl RasterRef for TestRaster {
- fn metadata(&self) -> &dyn MetadataRef {
- &self.metadata
+ fn num_bands(&self) -> usize {
+ 0
+ }
+ fn band(&self, _index: usize) -> Option<Box<dyn crate::traits::BandRef
+ '_>> {
+ None
+ }
+ fn band_name(&self, _index: usize) -> Option<&str> {
+ None
}
fn crs(&self) -> Option<&str> {
None
}
- fn bands(&self) -> &dyn crate::traits::BandsRef {
- unimplemented!()
+ fn transform(&self) -> &[f64] {
+ &self.transform
+ }
+ fn x_dim(&self) -> &str {
+ "x"
+ }
+ fn y_dim(&self) -> &str {
+ "y"
}
}
#[test]
fn test_rotation() {
- // 0 degree rotation -> gt[1.0, 0.0, 0.0, -1.0]
- let raster = rotation_raster(1.0, -1.0, 0.0, 0.0);
- let rot = rotation(&raster);
- assert_eq!(rot, 0.0);
-
- // pi/2 -> gt[0.0, -1.0, 1.0, 0.0]
- let raster = rotation_raster(0.0, 0.0, -1.0, 1.0);
- let rot = rotation(&raster);
- assert_relative_eq!(rot, PI / 2.0, epsilon = 1e-6); // 90 degrees in
radians
-
- // pi/4 -> gt[0.70710678, -0.70710678, 0.70710678, 0.70710678]
- let raster = rotation_raster(FRAC_1_SQRT_2, FRAC_1_SQRT_2,
-FRAC_1_SQRT_2, FRAC_1_SQRT_2);
- let rot = rotation(&raster);
- assert_relative_eq!(rot, PI / 4.0, epsilon = 1e-6); // 45 degrees in
radians
-
- // pi/3 -> gt[0.5, -0.866025, 0.866025, 0.5]
- let raster = rotation_raster(0.5, 0.5, -0.866025, 0.866025);
- let rot = rotation(&raster);
- assert_relative_eq!(rot, PI / 3.0, epsilon = 1e-6); // 60 degrees in
radians
-
- // pi -> gt[-1.0, 0.0, 0.0, -1.0]
- let raster = rotation_raster(-1.0, -1.0, 0.0, 0.0);
- let rot = rotation(&raster);
- assert_relative_eq!(rot, -PI, epsilon = 1e-6); // 180 degrees in
radians
+ // 0 degree rotation
+ let raster = TestRaster::new(0.0, 0.0, 1.0, -1.0, 0.0, 0.0);
+ assert_eq!(rotation(&raster), 0.0);
+
+ // pi/2
+ let raster = TestRaster::new(0.0, 0.0, 0.0, 0.0, -1.0, 1.0);
+ assert_relative_eq!(rotation(&raster), PI / 2.0, epsilon = 1e-6);
+
+ // pi/4
+ let raster = TestRaster::new(
+ 0.0,
+ 0.0,
+ FRAC_1_SQRT_2,
+ FRAC_1_SQRT_2,
+ -FRAC_1_SQRT_2,
+ FRAC_1_SQRT_2,
+ );
+ assert_relative_eq!(rotation(&raster), PI / 4.0, epsilon = 1e-6);
+
+ // pi/3
+ let raster = TestRaster::new(0.0, 0.0, 0.5, 0.5, -0.866025, 0.866025);
+ assert_relative_eq!(rotation(&raster), PI / 3.0, epsilon = 1e-6);
+
+ // pi
+ let raster = TestRaster::new(0.0, 0.0, -1.0, -1.0, 0.0, 0.0);
+ assert_relative_eq!(rotation(&raster), -PI, epsilon = 1e-6);
}
#[test]
fn test_to_world_coordinate() {
- // Test case with rotation/skew
- let raster = TestRaster {
- metadata: RasterMetadata {
- width: 10,
- height: 20,
- upperleft_x: 100.0,
- upperleft_y: 200.0,
- scale_x: 1.0,
- scale_y: -2.0,
- skew_x: 0.25,
- skew_y: 0.5,
- },
- };
-
- let (wx, wy) = to_world_coordinate(&raster, 0, 0);
- assert_eq!((wx, wy), (100.0, 200.0));
-
- let (wx, wy) = to_world_coordinate(&raster, 5, 10);
- assert_eq!((wx, wy), (107.5, 182.5));
+ let raster = TestRaster::new(100.0, 200.0, 1.0, -2.0, 0.25, 0.5);
- let (wx, wy) = to_world_coordinate(&raster, 9, 19);
- assert_eq!((wx, wy), (113.75, 166.5));
-
- let (wx, wy) = to_world_coordinate(&raster, 1, 0);
- assert_eq!((wx, wy), (101.0, 200.5));
-
- let (wx, wy) = to_world_coordinate(&raster, 0, 1);
- assert_eq!((wx, wy), (100.25, 198.0));
+ assert_eq!(to_world_coordinate(&raster, 0, 0), (100.0, 200.0));
+ assert_eq!(to_world_coordinate(&raster, 5, 10), (107.5, 182.5));
+ assert_eq!(to_world_coordinate(&raster, 9, 19), (113.75, 166.5));
+ assert_eq!(to_world_coordinate(&raster, 1, 0), (101.0, 200.5));
+ assert_eq!(to_world_coordinate(&raster, 0, 1), (100.25, 198.0));
}
#[test]
fn test_to_raster_coordinate() {
- // Test case with rotation/skew
- let raster = TestRaster {
- metadata: RasterMetadata {
- width: 10,
- height: 20,
- upperleft_x: 100.0,
- upperleft_y: 200.0,
- scale_x: 1.0,
- scale_y: -2.0,
- skew_x: 0.25,
- skew_y: 0.5,
- },
- };
-
- // Reverse of the to_world_coordinate tests
- let (wx, wy) = to_raster_coordinate(&raster, 100.0, 200.0).unwrap();
- assert_eq!((wx, wy), (0, 0));
-
- let (wx, wy) = to_raster_coordinate(&raster, 107.5, 182.5).unwrap();
- assert_eq!((wx, wy), (5, 10));
-
- let (wx, wy) = to_raster_coordinate(&raster, 113.75, 166.5).unwrap();
- assert_eq!((wx, wy), (9, 19));
-
- let (wx, wy) = to_raster_coordinate(&raster, 101.0, 200.5).unwrap();
- assert_eq!((wx, wy), (1, 0));
-
- let (wx, wy) = to_raster_coordinate(&raster, 100.25, 198.0).unwrap();
- assert_eq!((wx, wy), (0, 1));
-
- // Check error handling for zero determinant
- let bad_raster = TestRaster {
- metadata: RasterMetadata {
- width: 10,
- height: 20,
- upperleft_x: 100.0,
- upperleft_y: 200.0,
- scale_x: 1.0,
- scale_y: 0.0,
- skew_x: 0.0,
- skew_y: 0.0,
- },
- };
+ let raster = TestRaster::new(100.0, 200.0, 1.0, -2.0, 0.25, 0.5);
Review Comment:
For what it's worth the named struct construction here is more readable than
the revised `TestRaster::new()`
##########
rust/sedona-raster/src/affine_transformation.rs:
##########
@@ -324,11 +301,6 @@ mod tests {
};
let result = a.inv_transform(0.0, 0.0);
assert!(result.is_err());
- assert!(result
- .err()
- .unwrap()
- .to_string()
- .contains("determinant is zero."));
Review Comment:
Just checking that this expectation was moved and not deleted
##########
rust/sedona-raster/src/outdb_uri.rs:
##########
@@ -0,0 +1,133 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+/// Parsed components of an outdb_uri.
+///
+/// The outdb_uri format is `scheme://path#fragment`, e.g.:
+/// - `geotiff://s3://bucket/file.tif#band=1`
+/// - `zarr://s3://bucket/store#temperature/0.0.0`
+///
+/// The scheme determines which loader to dispatch to.
+/// The path is the external resource location (what RS_BandPath returns to
users).
+/// The fragment encodes loader-specific details (band id, chunk coords, etc.).
+/// Each loader defines its own fragment convention.
+///
+/// TODO: For formats like Zarr that may need complex metadata (array path,
chunk
+/// coordinates, byte ranges), a simple key-value fragment may not be
sufficient.
+/// If this becomes a limitation, consider switching the fragment to a JSON
object
+/// or making the entire outdb_uri a JSON string for those formats.
+#[derive(Debug, PartialEq)]
+pub struct OutDbUri<'a> {
+ /// Loader scheme (e.g., "geotiff", "zarr")
+ pub scheme: &'a str,
+ /// External resource path (e.g., "s3://bucket/file.tif")
+ pub path: &'a str,
+ /// Loader-specific fragment (e.g., "band=1"), or None if absent
+ pub fragment: Option<&'a str>,
+}
Review Comment:
There's a sturcture for this already that we probably want to reuse (I think
it's called the ObjectStoreUrl). It's based on the url crate I believe, which
would also be an improvement over rolling our own here. I think this would give
you the extra parameter information you'll need without having to maintain it
here.
##########
rust/sedona-testing/src/rasters.rs:
##########
@@ -578,65 +492,33 @@ mod tests {
fn test_raster_equal() {
let raster_array1 =
generate_tiled_rasters((256, 256), (1, 1), BandDataType::UInt8,
Some(43)).unwrap();
- let raster1 = RasterStructArray::new(&raster_array1).get(0).unwrap();
-
- // Assert that the rasters are equal to themselves
+ let rsa = RasterStructArray::new(&raster_array1);
+ let raster1 = rsa.get(0).unwrap();
assert_raster_equal(&raster1, &raster1);
}
#[test]
- #[should_panic = "Band data does not match"]
+ #[should_panic = "Band 0 data does not match"]
fn test_raster_different_band_data() {
let raster_array1 =
generate_tiled_rasters((128, 128), (1, 1), BandDataType::UInt8,
Some(43)).unwrap();
let raster_array2 =
generate_tiled_rasters((128, 128), (1, 1), BandDataType::UInt8,
Some(47)).unwrap();
-
- let raster1 = RasterStructArray::new(&raster_array1).get(0).unwrap();
- let raster2 = RasterStructArray::new(&raster_array2).get(0).unwrap();
+ let rsa1 = RasterStructArray::new(&raster_array1);
+ let rsa2 = RasterStructArray::new(&raster_array2);
+ let raster1 = rsa1.get(0).unwrap();
+ let raster2 = rsa2.get(0).unwrap();
assert_raster_equal(&raster1, &raster2);
}
#[test]
- fn test_generate_multi_band_raster() {
- let struct_array = generate_multi_band_raster();
- let raster_array = RasterStructArray::new(&struct_array);
- assert_eq!(raster_array.len(), 1);
Review Comment:
Is this test no longer relevant?
##########
rust/sedona-testing/src/rasters.rs:
##########
@@ -292,8 +215,6 @@ pub fn generate_multi_band_raster() -> StructArray {
builder.finish().unwrap()
}
-/// Determine if this tile contains a corner of the overall grid and return
its position
-/// Returns Some(position) if this tile contains a corner, None otherwise
Review Comment:
Is there a reason these comments were removed?
##########
rust/sedona-raster/src/display.rs:
##########
@@ -113,51 +93,3 @@ impl fmt::Display for RasterDisplay<'_> {
Ok(())
}
}
-
-#[cfg(test)]
-mod tests {
- use super::*;
- use crate::array::RasterStructArray;
- use sedona_testing::rasters::generate_test_rasters;
-
- #[test]
- fn display_non_skewed_raster() {
Review Comment:
Are these tests moved to a different place or should they be re-added?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
