Copilot commented on code in PR #2975:
URL: https://github.com/apache/sedona/pull/2975#discussion_r3270199823
##########
common/src/main/java/org/apache/sedona/common/S2Geography/WKBWriter.java:
##########
@@ -317,10 +317,15 @@ private void writePolygon(int geometryType,
PolygonGeography poly, OutStream os)
List<S2Loop> loops = s2poly.getLoops();
writeInt(loops.size(), os);
for (S2Loop loop : loops) {
+ // S2Loop stores N distinct vertices (loop is implicitly closed from
+ // vertex(n-1) back to vertex(0)). OGC WKB requires N+1 vertices with
+ // last == first. Emit the closing duplicate so downstream OGC-strict
+ // readers (e.g., geoarrow-c's WKB reader used by sedona-db's
+ // s2geography kernels) don't treat the ring as unclosed/degenerate.
int n = loop.numVertices();
- writeInt(n, os);
- for (int i = 0; i < n; i++) {
- S2LatLng ll = new S2LatLng(loop.vertex(i));
+ writeInt(n + 1, os);
+ for (int i = 0; i <= n; i++) {
+ S2LatLng ll = new S2LatLng(loop.vertex(i % n));
double lon = ll.lngDegrees();
Review Comment:
`loop.numVertices()` can be 0 for degenerate loops (S2 supports loops with
0/1/2 vertices; see usage like `new S2Loop(vertices)` where `vertices` may be
empty). The new `i % n` will throw `ArithmeticException` when `n == 0`, whereas
the previous implementation would safely write a 0-point ring. Please guard `n
== 0` (write 0 and skip coords) or avoid modulo by writing vertices 0..n-1 and
then explicitly duplicating vertex(0) when n>0.
##########
common/src/main/java/org/apache/sedona/common/S2Geography/WKBWriter.java:
##########
@@ -358,12 +363,14 @@ private void writeMultiPolygon(int geometryType,
MultiPolygonGeography multiPoly
List<S2Loop> loops = s2poly.polygon.getLoops();
writeInt(loops.size(), os);
- // 4c) For each ring, write vertex count + coords
+ // 4c) For each ring, write vertex count + coords.
+ // Emit N+1 vertices with last == first for OGC WKB compliance;
+ // see the explanatory comment in writePolygon().
for (S2Loop loop : loops) {
int n = loop.numVertices();
- writeInt(n, os);
- for (int i = 0; i < n; i++) {
- S2LatLng ll = new S2LatLng(loop.vertex(i));
+ writeInt(n + 1, os);
+ for (int i = 0; i <= n; i++) {
+ S2LatLng ll = new S2LatLng(loop.vertex(i % n));
double lon = ll.lngDegrees();
Review Comment:
Same `n == 0` regression as `writePolygon()`: `loop.vertex(i % n)` will
crash on empty/degenerate rings. Please handle `n == 0` before writing `n+1`
points, or write the closing point without modulo (write 0..n-1 then vertex(0)
if n>0).
--
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]
