You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2025/09/16 20:29:14 UTC
[PR] feat(python/sedonadb): Expose random_geometry as a Python function [sedona-db]
paleolimbot opened a new pull request, #97:
URL: https://github.com/apache/sedona-db/pull/97
We've been using the SQL table function `sd_random_geometry()` quite a bit for testing and benchmarking and I think this may make those types of simulations much nicer to write.
This still needs a bit of testing/moving of the parameter validation to Rust but it's not essential for release. I'll circle back after working on some more releasy-type tasks.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona): Improve sd_random_geometry() and expose as a Python function [sedona-db]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #97:
URL: https://github.com/apache/sedona-db/pull/97#discussion_r2683686518
##########
rust/sedona-testing/src/datagen.rs:
##########
@@ -733,26 +818,44 @@ fn generate_random_circle<R: rand::Rng>(
rng: &mut R,
options: &RandomGeometryOptions,
) -> Result<(f64, f64, f64)> {
- // Generate random diamond polygons (rotated squares)
- let size_dist = Uniform::new(options.size_range.0, options.size_range.1)
- .map_err(|e| DataFusionError::External(Box::new(e)))?;
- let half_size = rng.sample(size_dist) / 2.0;
+ // Generate random circular polygons
+ let size_dist = Uniform::new_inclusive(options.size_range.0, options.size_range.1)
+ .map_err(|e| exec_datafusion_err!("Invalid size range for random region: {e}"))?;
+ let size = rng.sample(size_dist);
+ let half_size = size / 2.0;
+ let height = options.bounds.height();
+ let width = options.bounds.width();
// Ensure diamond fits within bounds by constraining center position
Review Comment:
The comment references 'diamond' but the function is generating circular polygons. Update the comment to say 'Ensure circle fits within bounds by constraining center position'.
```suggestion
// Ensure circle fits within bounds by constraining center position
```
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python/sedonadb): Expose random_geometry as a Python function [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #97:
URL: https://github.com/apache/sedona-db/pull/97#issuecomment-3457814281
Thank you for the review! I will try to circle back to this in the next few days. One of the things I discovered here is that our random geometry generator can panic for some parameter combinations, which needed fixing at a lower level 😬
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona): Improve sd_random_geometry() and expose as a Python function [sedona-db]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #97:
URL: https://github.com/apache/sedona-db/pull/97
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python/sedonadb): Expose random_geometry as a Python function [sedona-db]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #97:
URL: https://github.com/apache/sedona-db/pull/97#discussion_r2464752604
##########
python/sedonadb/python/sedonadb/testing.py:
##########
@@ -29,6 +29,85 @@
import sedonadb
+def random_geometry(
+ n: int = 1024,
+ geometry_type: Literal[
+ "Point",
+ "LineString",
+ "Polygon",
+ "MultiPoint",
+ "MultiLineString",
+ "MultiPolygon",
+ "GeometryCollection",
+ ] = "Point",
+ *,
+ num_vertices: Union[int, Tuple[int, int]] = 4,
+ num_parts: Union[int, Tuple[int, int]] = (1, 3),
+ size: Union[float, Tuple[float, float]] = (5.0, 20.0),
+ bounds: Iterable[float] = (-170, -80, 170, 80),
+ hole_rate: float = 0.0,
+ empty_rate: float = 0.0,
+ null_rate: float = 0.0,
+ seed: Optional[int] = None,
+) -> "sedonadb.dataframe.DataFrame":
Review Comment:
nit: Missing docstring
##########
python/sedonadb/python/sedonadb/testing.py:
##########
@@ -29,6 +29,85 @@
import sedonadb
+def random_geometry(
+ n: int = 1024,
+ geometry_type: Literal[
+ "Point",
+ "LineString",
+ "Polygon",
+ "MultiPoint",
+ "MultiLineString",
+ "MultiPolygon",
+ "GeometryCollection",
+ ] = "Point",
+ *,
+ num_vertices: Union[int, Tuple[int, int]] = 4,
+ num_parts: Union[int, Tuple[int, int]] = (1, 3),
+ size: Union[float, Tuple[float, float]] = (5.0, 20.0),
+ bounds: Iterable[float] = (-170, -80, 170, 80),
+ hole_rate: float = 0.0,
+ empty_rate: float = 0.0,
+ null_rate: float = 0.0,
+ seed: Optional[int] = None,
+) -> "sedonadb.dataframe.DataFrame":
+ import json
+ import time
+
+ import sedonadb
+
+ if isinstance(num_vertices, tuple):
+ num_vertices_min, num_vertices_max = num_vertices
+ else:
+ num_vertices_min = num_vertices_max = num_vertices
+
+ if isinstance(num_parts, tuple):
+ num_parts_min, num_parts_max = num_parts
+ else:
+ num_parts_min = num_parts_max = num_parts
+
+ if isinstance(size, tuple):
+ size_min, size_max = size
+ else:
+ size_min = size
+ size_max = size + size / 1e3
+
+ if num_vertices_min > num_vertices_max:
+ raise ValueError("num_vertices_min > num_vertices_max")
+ if num_parts_min > num_parts_max:
+ raise ValueError("num_parts_min > num_parts_max")
+ if size_min > size_max:
+ raise ValueError("size_min > size_max")
+
+ bounds = [float(b) for b in bounds]
+ if len(bounds) != 4:
+ raise ValueError(
+ f"Expected bounds as [xmin, ymin, xmax, ymax] but got {bounds}"
+ )
+
+ width = bounds[2] - bounds[0]
+ height = bounds[3] - bounds[1]
+ if size_min > width or size_min > height:
+ raise ValueError("size > height / 2 or width / 2 of bounds")
Review Comment:
The error message does not match the check. The error message talks about halfs.
##########
python/sedonadb/python/sedonadb/testing.py:
##########
@@ -29,6 +29,85 @@
import sedonadb
+def random_geometry(
+ n: int = 1024,
+ geometry_type: Literal[
+ "Point",
+ "LineString",
+ "Polygon",
+ "MultiPoint",
+ "MultiLineString",
+ "MultiPolygon",
+ "GeometryCollection",
+ ] = "Point",
+ *,
+ num_vertices: Union[int, Tuple[int, int]] = 4,
+ num_parts: Union[int, Tuple[int, int]] = (1, 3),
+ size: Union[float, Tuple[float, float]] = (5.0, 20.0),
+ bounds: Iterable[float] = (-170, -80, 170, 80),
+ hole_rate: float = 0.0,
+ empty_rate: float = 0.0,
+ null_rate: float = 0.0,
+ seed: Optional[int] = None,
+) -> "sedonadb.dataframe.DataFrame":
+ import json
+ import time
+
+ import sedonadb
+
+ if isinstance(num_vertices, tuple):
+ num_vertices_min, num_vertices_max = num_vertices
+ else:
+ num_vertices_min = num_vertices_max = num_vertices
+
+ if isinstance(num_parts, tuple):
+ num_parts_min, num_parts_max = num_parts
+ else:
+ num_parts_min = num_parts_max = num_parts
+
+ if isinstance(size, tuple):
+ size_min, size_max = size
+ else:
+ size_min = size
+ size_max = size + size / 1e3
+
+ if num_vertices_min > num_vertices_max:
+ raise ValueError("num_vertices_min > num_vertices_max")
+ if num_parts_min > num_parts_max:
+ raise ValueError("num_parts_min > num_parts_max")
+ if size_min > size_max:
+ raise ValueError("size_min > size_max")
+
+ bounds = [float(b) for b in bounds]
+ if len(bounds) != 4:
+ raise ValueError(
+ f"Expected bounds as [xmin, ymin, xmax, ymax] but got {bounds}"
+ )
+
+ width = bounds[2] - bounds[0]
Review Comment:
Should there be checks that xmin is smaller than xmax ?
Same for the `y` bounds below.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(python/sedonadb): Expose random_geometry as a Python function [sedona-db]
Posted by "petern48 (via GitHub)" <gi...@apache.org>.
petern48 commented on PR #97:
URL: https://github.com/apache/sedona-db/pull/97#issuecomment-3300854549
This reminds me that when I was writing up the pytest-benchmarking code, I realized it would be nice to have a random geometry rust function (instead of a table provider) that returns a single column, so that we could create multiple unique random geometry columns in a single query to use with functions that take two geometry inputs (e.g predicates). I can't think of many other use cases tho, so it doesn't seem very high priority.
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust/sedona): Improve sd_random_geometry() and expose as a Python function [sedona-db]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #97:
URL: https://github.com/apache/sedona-db/pull/97#discussion_r2679287130
##########
rust/sedona-testing/src/datagen.rs:
##########
@@ -490,6 +522,52 @@ impl RandomGeometryOptions {
num_parts_range: (1, 3),
}
}
+
+ fn validate(&self) -> Result<()> {
+ if self.bounds.width() <= 0.0 || self.bounds.height() <= 0.0 {
+ return plan_err!("Expected valid bounds but got {:?}", self.bounds);
+ }
+
+ if self.bounds.width() <= 0.0 || self.bounds.height() <= 0.0 {
+ return plan_err!("Expected valid bounds but got {:?}", self.bounds);
+ }
+
Review Comment:
Duplicate validation logic for bounds. Lines 527-529 and 531-533 check the exact same condition. Remove one of these duplicate checks.
```suggestion
```
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org