Copilot commented on code in PR #2257:
URL: https://github.com/apache/sedona/pull/2257#discussion_r2264170957
##########
common/src/main/java/org/apache/sedona/common/S2Geography/MultiPolygonGeography.java:
##########
@@ -21,18 +21,14 @@
import com.google.common.geometry.S2Polygon;
import java.util.Collections;
import java.util.List;
-import java.util.stream.Collectors;
public class MultiPolygonGeography extends GeographyCollection {
/**
* Wrap each raw S2Polygon in a PolygonGeography, then hand it off to
GeographyCollection to do
* the rest (including serialization).
*/
- public MultiPolygonGeography(List<S2Polygon> polygons) {
- super(
- polygons.stream()
- .map(PolygonGeography::new) // wrap each S2Polygon
- .collect(Collectors.toList())); // into a List<PolygonGeography>
+ public MultiPolygonGeography(GeographyKind kind, List<S2Polygon> polygons) {
+ super(kind, polygons);
if (polygons.isEmpty()) {
new MultiPolygonGeography();
Review Comment:
This line creates a new MultiPolygonGeography object but doesn't assign it
to anything or return it. This appears to be a bug - either it should be
removed or the logic needs to be corrected.
```suggestion
```
##########
common/src/main/java/org/apache/sedona/common/S2Geography/Geography.java:
##########
@@ -243,6 +244,7 @@ public static Geography decode(UnsafeInput in, EncodeTag
tag) throws IOException
geo = PolylineGeography.decode(in, tag);
break;
case POLYGON:
+ case MULTIPOLYGON:
geo = PolygonGeography.decode(in, tag);
break;
Review Comment:
MULTIPOLYGON case falls through to POLYGON handling, but
MultiPolygonGeography should be decoded differently than PolygonGeography. This
could lead to incorrect deserialization behavior.
```suggestion
geo = PolygonGeography.decode(in, tag);
break;
case MULTIPOLYGON:
geo = MultiPolygonGeography.decode(in, tag);
break;
```
##########
common/src/main/java/org/apache/sedona/common/S2Geography/GeographyCollection.java:
##########
@@ -44,6 +45,21 @@ public GeographyCollection() {
this.totalShapes = 0;
}
+ public GeographyCollection(GeographyKind kind, List<S2Polygon> polygons) {
+ super(kind); // can be MULTIPOLYGON
+ if (kind != GeographyKind.MULTIPOLYGON) {
+ throw new IllegalArgumentException(
+ "Invalid GeographyKind, only allow Multipolygon as in
geographyCollection: " + kind);
+ }
Review Comment:
The validation logic is restrictive and only allows MULTIPOLYGON kind, but
the constructor parameter name 'kind' suggests it should be more flexible.
Consider renaming the parameter to 'multiPolygonKind' or documenting why only
MULTIPOLYGON is allowed.
```suggestion
/**
* Constructs a GeographyCollection from a list of S2Polygons.
* This constructor always creates a MULTIPOLYGON GeographyCollection.
*/
public GeographyCollection(List<S2Polygon> polygons) {
super(GeographyKind.MULTIPOLYGON);
```
--
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]
