Copilot commented on code in PR #466:
URL: https://github.com/apache/sedona-db/pull/466#discussion_r2660223807
##########
rust/sedona-testing/src/datagen.rs:
##########
@@ -511,167 +514,178 @@ fn generate_random_wkb<R: rand::Rng>(rng: &mut R,
options: &RandomGeometryOption
endianness: Endianness::LittleEndian,
},
)
- .unwrap();
- out
+ .map_err(|e| DataFusionError::External(Box::new(e)))?;
+ Ok(out)
}
fn generate_random_geometry<R: rand::Rng>(
rng: &mut R,
options: &RandomGeometryOptions,
-) -> Geometry {
- match options.geom_type {
- GeometryTypeId::Point => Geometry::Point(generate_random_point(rng,
options)),
+) -> Result<Geometry> {
+ Ok(match options.geom_type {
+ GeometryTypeId::Point => Geometry::Point(generate_random_point(rng,
options)?),
GeometryTypeId::LineString => {
- Geometry::LineString(generate_random_linestring(rng, options))
+ Geometry::LineString(generate_random_linestring(rng, options)?)
}
- GeometryTypeId::Polygon =>
Geometry::Polygon(generate_random_polygon(rng, options)),
+ GeometryTypeId::Polygon =>
Geometry::Polygon(generate_random_polygon(rng, options)?),
GeometryTypeId::MultiPoint => {
- Geometry::MultiPoint(generate_random_multipoint(rng, options))
+ Geometry::MultiPoint(generate_random_multipoint(rng, options)?)
}
GeometryTypeId::MultiLineString => {
- Geometry::MultiLineString(generate_random_multilinestring(rng,
options))
+ Geometry::MultiLineString(generate_random_multilinestring(rng,
options)?)
}
GeometryTypeId::MultiPolygon => {
- Geometry::MultiPolygon(generate_random_multipolygon(rng, options))
+ Geometry::MultiPolygon(generate_random_multipolygon(rng, options)?)
}
GeometryTypeId::GeometryCollection => {
-
Geometry::GeometryCollection(generate_random_geometrycollection(rng, options))
+
Geometry::GeometryCollection(generate_random_geometrycollection(rng, options)?)
}
GeometryTypeId::Geometry => {
let mut copy_options = options.clone();
copy_options.geom_type = pick_random_geometry_type(rng);
- generate_random_geometry(rng, ©_options)
+ generate_random_geometry(rng, ©_options)?
}
- }
+ })
}
-fn generate_random_point<R: rand::Rng>(rng: &mut R, options:
&RandomGeometryOptions) -> Point {
- if rng.gen_bool(options.empty_rate) {
+fn generate_random_point<R: rand::Rng>(
+ rng: &mut R,
+ options: &RandomGeometryOptions,
+) -> Result<Point> {
+ if rng.random_bool(options.empty_rate) {
// This is a bit of a hack because geo-types doesn't support empty
point; however,
// this does work with respect to sending this directly to the WKB
reader and getting
// the WKB result we want
- Point::new(f64::NAN, f64::NAN)
+ Ok(Point::new(f64::NAN, f64::NAN))
} else {
// Generate random points within the specified bounds
- let x_dist = Uniform::new(options.bounds.min().x,
options.bounds.max().x);
- let y_dist = Uniform::new(options.bounds.min().y,
options.bounds.max().y);
+ let x_dist = Uniform::new(options.bounds.min().x,
options.bounds.max().x)
+ .map_err(|e| DataFusionError::External(Box::new(e)))?;
+ let y_dist = Uniform::new(options.bounds.min().y,
options.bounds.max().y)
+ .map_err(|e| DataFusionError::External(Box::new(e)))?;
let x = rng.sample(x_dist);
let y = rng.sample(y_dist);
- Point::new(x, y)
+ Ok(Point::new(x, y))
}
}
fn generate_random_linestring<R: rand::Rng>(
rng: &mut R,
options: &RandomGeometryOptions,
-) -> LineString {
- if rng.gen_bool(options.empty_rate) {
- LineString::new(vec![])
+) -> Result<LineString> {
+ if rng.random_bool(options.empty_rate) {
+ Ok(LineString::new(vec![]))
} else {
- let (center_x, center_y, half_size) = generate_random_circle(rng,
options);
+ let (center_x, center_y, half_size) = generate_random_circle(rng,
options)?;
let vertices_dist = Uniform::new_inclusive(
options.vertices_per_linestring_range.0,
options.vertices_per_linestring_range.1,
- );
+ )
+ .unwrap();
Review Comment:
Using unwrap() here will panic with a generic message if
Uniform::new_inclusive fails. Since other Uniform::new calls in this file use
expect() with descriptive messages (e.g., line 615 'Valid input range'), these
should also use expect() for consistency and better error messages, such as
'.expect(\"valid vertices_per_linestring_range\")'.
```suggestion
.expect("valid vertices_per_linestring_range");
```
##########
rust/sedona-testing/src/benchmark_util.rs:
##########
@@ -424,7 +424,7 @@ impl BenchmarkArgSpec {
}
BenchmarkArgSpec::Int32(lo, hi) => {
let mut rng = self.rng(i);
- let dist = Uniform::new(lo, hi);
+ let dist = Uniform::new(lo, hi).unwrap();
Review Comment:
Using unwrap() here will panic with a generic message if the range is
invalid. Consider using expect() with a descriptive message such as
'.expect(\"valid range for Uniform distribution\")' to provide better error
context when Uniform::new fails.
##########
rust/sedona-testing/src/benchmark_util.rs:
##########
@@ -402,7 +402,7 @@ impl BenchmarkArgSpec {
),
BenchmarkArgSpec::Int64(lo, hi) => {
let mut rng = self.rng(i);
- let dist = Uniform::new(lo, hi);
+ let dist = Uniform::new(lo, hi).unwrap();
Review Comment:
Using unwrap() here will panic with a generic message if the range is
invalid. Consider using expect() with a descriptive message such as
'.expect(\"valid range for Uniform distribution\")' to provide better error
context when Uniform::new fails.
##########
rust/sedona-testing/src/datagen.rs:
##########
@@ -511,167 +514,178 @@ fn generate_random_wkb<R: rand::Rng>(rng: &mut R,
options: &RandomGeometryOption
endianness: Endianness::LittleEndian,
},
)
- .unwrap();
- out
+ .map_err(|e| DataFusionError::External(Box::new(e)))?;
+ Ok(out)
}
fn generate_random_geometry<R: rand::Rng>(
rng: &mut R,
options: &RandomGeometryOptions,
-) -> Geometry {
- match options.geom_type {
- GeometryTypeId::Point => Geometry::Point(generate_random_point(rng,
options)),
+) -> Result<Geometry> {
+ Ok(match options.geom_type {
+ GeometryTypeId::Point => Geometry::Point(generate_random_point(rng,
options)?),
GeometryTypeId::LineString => {
- Geometry::LineString(generate_random_linestring(rng, options))
+ Geometry::LineString(generate_random_linestring(rng, options)?)
}
- GeometryTypeId::Polygon =>
Geometry::Polygon(generate_random_polygon(rng, options)),
+ GeometryTypeId::Polygon =>
Geometry::Polygon(generate_random_polygon(rng, options)?),
GeometryTypeId::MultiPoint => {
- Geometry::MultiPoint(generate_random_multipoint(rng, options))
+ Geometry::MultiPoint(generate_random_multipoint(rng, options)?)
}
GeometryTypeId::MultiLineString => {
- Geometry::MultiLineString(generate_random_multilinestring(rng,
options))
+ Geometry::MultiLineString(generate_random_multilinestring(rng,
options)?)
}
GeometryTypeId::MultiPolygon => {
- Geometry::MultiPolygon(generate_random_multipolygon(rng, options))
+ Geometry::MultiPolygon(generate_random_multipolygon(rng, options)?)
}
GeometryTypeId::GeometryCollection => {
-
Geometry::GeometryCollection(generate_random_geometrycollection(rng, options))
+
Geometry::GeometryCollection(generate_random_geometrycollection(rng, options)?)
}
GeometryTypeId::Geometry => {
let mut copy_options = options.clone();
copy_options.geom_type = pick_random_geometry_type(rng);
- generate_random_geometry(rng, ©_options)
+ generate_random_geometry(rng, ©_options)?
}
- }
+ })
}
-fn generate_random_point<R: rand::Rng>(rng: &mut R, options:
&RandomGeometryOptions) -> Point {
- if rng.gen_bool(options.empty_rate) {
+fn generate_random_point<R: rand::Rng>(
+ rng: &mut R,
+ options: &RandomGeometryOptions,
+) -> Result<Point> {
+ if rng.random_bool(options.empty_rate) {
// This is a bit of a hack because geo-types doesn't support empty
point; however,
// this does work with respect to sending this directly to the WKB
reader and getting
// the WKB result we want
- Point::new(f64::NAN, f64::NAN)
+ Ok(Point::new(f64::NAN, f64::NAN))
} else {
// Generate random points within the specified bounds
- let x_dist = Uniform::new(options.bounds.min().x,
options.bounds.max().x);
- let y_dist = Uniform::new(options.bounds.min().y,
options.bounds.max().y);
+ let x_dist = Uniform::new(options.bounds.min().x,
options.bounds.max().x)
+ .map_err(|e| DataFusionError::External(Box::new(e)))?;
+ let y_dist = Uniform::new(options.bounds.min().y,
options.bounds.max().y)
+ .map_err(|e| DataFusionError::External(Box::new(e)))?;
let x = rng.sample(x_dist);
let y = rng.sample(y_dist);
- Point::new(x, y)
+ Ok(Point::new(x, y))
}
}
fn generate_random_linestring<R: rand::Rng>(
rng: &mut R,
options: &RandomGeometryOptions,
-) -> LineString {
- if rng.gen_bool(options.empty_rate) {
- LineString::new(vec![])
+) -> Result<LineString> {
+ if rng.random_bool(options.empty_rate) {
+ Ok(LineString::new(vec![]))
} else {
- let (center_x, center_y, half_size) = generate_random_circle(rng,
options);
+ let (center_x, center_y, half_size) = generate_random_circle(rng,
options)?;
let vertices_dist = Uniform::new_inclusive(
options.vertices_per_linestring_range.0,
options.vertices_per_linestring_range.1,
- );
+ )
+ .unwrap();
// Always sample in such a way that we end up with a valid linestring
let num_vertices = rng.sample(vertices_dist).max(2);
let coords =
- generate_circular_vertices(rng, center_x, center_y, half_size,
num_vertices, false);
- LineString::from(coords)
+ generate_circular_vertices(rng, center_x, center_y, half_size,
num_vertices, false)?;
+ Ok(LineString::from(coords))
}
}
-fn generate_random_polygon<R: rand::Rng>(rng: &mut R, options:
&RandomGeometryOptions) -> Polygon {
- if rng.gen_bool(options.empty_rate) {
- Polygon::new(LineString::new(vec![]), vec![])
+fn generate_random_polygon<R: rand::Rng>(
+ rng: &mut R,
+ options: &RandomGeometryOptions,
+) -> Result<Polygon> {
+ if rng.random_bool(options.empty_rate) {
+ Ok(Polygon::new(LineString::new(vec![]), vec![]))
} else {
- let (center_x, center_y, half_size) = generate_random_circle(rng,
options);
+ let (center_x, center_y, half_size) = generate_random_circle(rng,
options)?;
let vertices_dist = Uniform::new_inclusive(
options.vertices_per_linestring_range.0,
options.vertices_per_linestring_range.1,
- );
+ )
+ .unwrap();
Review Comment:
Using unwrap() here will panic with a generic message if
Uniform::new_inclusive fails. Since other Uniform::new calls in this file use
expect() with descriptive messages (e.g., line 615 'Valid input range'), these
should also use expect() for consistency and better error messages, such as
'.expect(\"valid vertices_per_linestring_range\")'.
##########
rust/sedona/src/context.rs:
##########
@@ -509,12 +509,14 @@ struct ThreadSafeDialect {
unsafe impl Send for ThreadSafeDialect {}
impl ThreadSafeDialect {
- pub fn try_new(dialect_str: &str) -> Result<Self> {
- let dialect = dialect_from_str(dialect_str)
- .ok_or_else(|| plan_datafusion_err!("Unsupported SQL dialect:
{dialect_str}"))?;
- Ok(Self {
- inner: dialect.into(),
- })
+ pub fn try_new(dialect: &datafusion::config::Dialect) -> Result<Self> {
+ if let Some(box_dialect) = dialect_from_str(dialect) {
+ Ok(Self {
+ inner: box_dialect.into(),
+ })
+ } else {
+ plan_err!("Unexpected dialect: {dialect}")
Review Comment:
The error message 'Unexpected dialect' is unclear since this code path is
reached when dialect_from_str returns None, not when an unexpected dialect is
provided. Consider changing to 'Unsupported SQL dialect' or 'Failed to parse
SQL dialect' to better reflect the actual error condition.
```suggestion
plan_err!("Unsupported SQL dialect: {dialect}")
```
##########
rust/sedona-testing/src/benchmark_util.rs:
##########
@@ -413,7 +413,7 @@ impl BenchmarkArgSpec {
}
BenchmarkArgSpec::Float64(lo, hi) => {
let mut rng = self.rng(i);
- let dist = Uniform::new(lo, hi);
+ let dist = Uniform::new(lo, hi).unwrap();
Review Comment:
Using unwrap() here will panic with a generic message if the range is
invalid. Consider using expect() with a descriptive message such as
'.expect(\"valid range for Uniform distribution\")' to provide better error
context when Uniform::new fails.
--
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]
