Copilot commented on code in PR #2811:
URL: https://github.com/apache/sedona/pull/2811#discussion_r3020198670
##########
common/src/main/java/org/apache/sedona/common/Functions.java:
##########
@@ -424,6 +425,20 @@ else if
(singleParam[0].equalsIgnoreCase(listBufferParameters[5])) {
return bufferParameters;
}
+ public static Geometry offsetCurve(Geometry geometry, double distance) {
+ return offsetCurve(geometry, distance,
BufferParameters.DEFAULT_QUADRANT_SEGMENTS);
+ }
+
+ public static Geometry offsetCurve(Geometry geometry, double distance, int
quadrantSegments) {
+ if (geometry.isEmpty()) {
+ return null;
+ }
+ BufferParameters bufferParameters = new BufferParameters();
+ bufferParameters.setQuadrantSegments(quadrantSegments);
+ OffsetCurve oc = new OffsetCurve(geometry, distance, bufferParameters);
+ return oc.getCurve();
Review Comment:
`offsetCurve(Geometry, double, int)` calls `geometry.isEmpty()` without a
null guard, which will throw `NullPointerException` if this method is invoked
directly with a null geometry (unlike Spark SQL expressions which short-circuit
on null args). Consider aligning with nearby helpers (e.g., `expand`) by
returning null when `geometry == null`, before checking `isEmpty()`.
##########
snowflake-tester/src/test/java/org/apache/sedona/snowflake/snowsql/TestFunctionsV2.java:
##########
@@ -716,6 +716,19 @@ public void test_ST_NPoints() {
"select sedona.ST_NPoints(ST_GeometryFromWKT('LINESTRING(1 2, 3 4, 5
6)'))", 3);
}
+ @Test
+ public void test_ST_OffsetCurve() {
+ registerUDFV2("ST_OffsetCurve", String.class, double.class);
+ verifySqlSingleRes(
+ "select
sedona.ST_AsText(sedona.ST_OffsetCurve(ST_GeometryFromWKT('LINESTRING(0 0, 10
0)'), 5.0))",
+ "LINESTRING (0 5, 10 5)");
+ registerUDFV2("ST_OffsetCurve", String.class, double.class, int.class);
+ registerUDFV2("ST_NPoints", String.class);
+ verifySqlSingleRes(
+ "select
sedona.ST_NPoints(sedona.ST_OffsetCurve(ST_GeometryFromWKT('LINESTRING(0 0, 10
0, 10 10)'), -3.0, 16))",
+ 19);
Review Comment:
This test asserts an exact `ST_NPoints` value (19) for an offset curve with
`quadrantSegments=16`. The vertex count of `OffsetCurve` results is
algorithm/version dependent and can change across JTS updates, which makes the
test fragile. Consider comparing against the default `ST_OffsetCurve(geom,
distance)` point count instead (asserting the custom count is greater), or
otherwise loosening the assertion to reflect the intended behavior.
```suggestion
"select
sedona.ST_NPoints(sedona.ST_OffsetCurve(ST_GeometryFromWKT('LINESTRING(0 0, 10
0, 10 10)'), -3.0, 16)) > "
+
"sedona.ST_NPoints(sedona.ST_OffsetCurve(ST_GeometryFromWKT('LINESTRING(0 0, 10
0, 10 10)'), -3.0))",
true);
```
##########
snowflake-tester/src/test/java/org/apache/sedona/snowflake/snowsql/TestFunctions.java:
##########
@@ -768,6 +768,19 @@ public void test_ST_NPoints() {
"select sedona.ST_NPoints(sedona.ST_GeomFromText('LINESTRING(1 2, 3 4,
5 6)'))", 3);
}
+ @Test
+ public void test_ST_OffsetCurve() {
+ registerUDF("ST_OffsetCurve", byte[].class, double.class);
+ verifySqlSingleRes(
+ "select
sedona.ST_AsText(sedona.ST_OffsetCurve(sedona.ST_GeomFromText('LINESTRING(0 0,
10 0)'), 5.0))",
+ "LINESTRING (0 5, 10 5)");
+ registerUDF("ST_OffsetCurve", byte[].class, double.class, int.class);
+ registerUDF("ST_NPoints", byte[].class);
+ verifySqlSingleRes(
+ "select
sedona.ST_NPoints(sedona.ST_OffsetCurve(sedona.ST_GeomFromText('LINESTRING(0 0,
10 0, 10 10)'), -3.0, 16))",
+ 19);
Review Comment:
This test asserts an exact `ST_NPoints` value (19) for an offset curve with
`quadrantSegments=16`. The number of vertices in `OffsetCurve` output can
change across JTS/Sedona upgrades even when the geometry is still correct,
making the test brittle. Prefer asserting relative behavior (e.g., `NPoints`
with `quadrantSegments=16` is greater than the default call) or validating the
curve shape in a less version-sensitive way.
```suggestion
"select iff(" +
"
sedona.ST_NPoints(sedona.ST_OffsetCurve(sedona.ST_GeomFromText('LINESTRING(0 0,
10 0, 10 10)'), -3.0, 16)) > " +
"
sedona.ST_NPoints(sedona.ST_OffsetCurve(sedona.ST_GeomFromText('LINESTRING(0 0,
10 0, 10 10)'), -3.0))," +
" 1, 0)",
1);
```
--
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]
