Copilot commented on code in PR #2027: URL: https://github.com/apache/sedona/pull/2027#discussion_r2195739777
########## common/src/main/java/org/apache/sedona/common/S2Geography/GeographyCollection.java: ########## @@ -0,0 +1,147 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sedona.common.S2Geography; + +import com.esotericsoftware.kryo.io.UnsafeInput; +import com.esotericsoftware.kryo.io.UnsafeOutput; +import com.google.common.collect.ImmutableList; +import com.google.common.geometry.*; +import java.io.*; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.logging.Logger; + +/** A Geography wrapping zero or more Geography objects, representing a GEOMETRYCOLLECTION. */ +public class GeographyCollection extends S2Geography { + private static final Logger logger = Logger.getLogger(GeographyCollection.class.getName()); + + public final List<S2Geography> features; + public final List<Integer> numShapesList; + public int totalShapes; + + /** Constructs an empty GeographyCollection. */ + public GeographyCollection() { + super(GeographyKind.GEOGRAPHY_COLLECTION); + this.features = new ArrayList<>(); + this.numShapesList = new ArrayList<>(); + this.totalShapes = 0; + } + + /** Wraps existing Geography features. */ + public GeographyCollection(List<S2Geography> features) { + super(GeographyKind.GEOGRAPHY_COLLECTION); + this.features = new ArrayList<>(features); + this.numShapesList = new ArrayList<>(); + this.totalShapes = 0; + countShapes(); + } + + @Override + public int dimension() { + // Mixed or empty → return -1; uniform → return 0,1,2 + return computeDimensionFromShapes(); + } + + @Override + public int numShapes() { + return totalShapes; + } + + @Override + public S2Shape shape(int id) { + int sum = 0; + for (int i = 0; i < features.size(); i++) { + int n = numShapesList.get(i); + sum += n; + if (id < sum) { + // index within this feature + return features.get(i).shape(id - (sum - n)); + } + } + throw new IllegalArgumentException("Shape id out of bounds: " + id); + } + + @Override + public S2Region region() { + Collection<S2Region> regs = new ArrayList<>(); + for (S2Geography geo : features) { + regs.add(geo.region()); + } + return new S2RegionUnion(regs); + } + + /** Returns an immutable copy of the features list. */ + public List<S2Geography> getFeatures() { + return ImmutableList.copyOf(features); + } + + @Override + public void encode(UnsafeOutput out, EncodeOptions opts) throws IOException { + // Top-level collection encodes its size and then each child with tagging + // Never include coverings for children (only a top-level concept) + EncodeOptions child_options = opts; Review Comment: [nitpick] Mutating the passed-in EncodeOptions may have side effects on the caller. Consider cloning the options (e.g., new EncodeOptions(opts)) before modifying includeCovering to preserve immutability. ```suggestion EncodeOptions child_options = new EncodeOptions(opts); ``` ########## common/src/main/java/org/apache/sedona/common/S2Geography/ShapeIndexGeography.java: ########## @@ -0,0 +1,119 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sedona.common.S2Geography; + +import com.esotericsoftware.kryo.io.Input; +import com.esotericsoftware.kryo.io.Output; +import com.esotericsoftware.kryo.io.UnsafeInput; +import com.esotericsoftware.kryo.io.UnsafeOutput; +import com.google.common.geometry.*; +import java.io.ByteArrayOutputStream; +import java.io.IOException; + +public class ShapeIndexGeography extends S2Geography { + public S2ShapeIndex shapeIndex; + + /** Build an empty ShapeIndexGeography. */ + public ShapeIndexGeography() { + super(GeographyKind.SHAPE_INDEX); + this.shapeIndex = new S2ShapeIndex(); + } + + /** Build and immediately add one Geography. */ + public ShapeIndexGeography(S2Geography geog) { + super(GeographyKind.SHAPE_INDEX); + this.shapeIndex = new S2ShapeIndex(); + addIndex(geog); + } + + /** Create a ShapeIndexGeography with a custom max-edges-per-cell. */ + public ShapeIndexGeography(int maxEdgesPerCell) { + super(GeographyKind.SHAPE_INDEX); + S2ShapeIndex.Options options = new S2ShapeIndex.Options(); + options.setMaxEdgesPerCell(maxEdgesPerCell); + this.shapeIndex = new S2ShapeIndex(options); + } + + @Override + public int dimension() { + return -1; + } + + @Override + public int numShapes() { + return shapeIndex.getShapes().size(); + } + + @Override + public S2Shape shape(int id) { + S2Shape raw = shapeIndex.getShapes().get(id); + return raw; + } + + @Override + public S2Region region() { + return new S2ShapeIndexRegion(shapeIndex); + } + /** Index every S2Shape from the given Geography. */ + public void addIndex(S2Geography geog) { + for (int i = 0, n = geog.numShapes(); i < n; i++) { + shapeIndex.add(geog.shape(i)); + } + } + + // encode + @Override + public void encode(UnsafeOutput out, EncodeOptions opts) throws IOException { + // 0) Prepare a temporary output for the payload + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + Output tmpOut = new Output(baos); + switch (opts.getCodingHint()) { + case FAST: + VectorCoder.FAST_SHAPE.encode(shapeIndex.getShapes(), tmpOut); + break; + case COMPACT: + VectorCoder.COMPACT_SHAPE.encode(shapeIndex.getShapes(), tmpOut); + } + // 2) Finish payload + tmpOut.flush(); + byte[] payload = baos.toByteArray(); + + // 3) Write length-prefix + payload to the real out + out.writeInt(payload.length, /* optimizePositive= */ false); + out.writeBytes(payload); + + // 4) Encode the index’s quadtree structure and flush + S2ShapeIndexCoder.INSTANCE.encode(shapeIndex, out); + out.flush(); + } + + // decode + /** This is what decodeTagged() actually calls */ + public static ShapeIndexGeography decode(Input in, EncodeTag tag) throws IOException { + // cast to UnsafeInput—will work if you always pass a Kryo-backed stream + if (!(in instanceof UnsafeInput)) { + throw new IllegalArgumentException("Expected UnsafeInput"); + } + return decode((UnsafeInput) in, tag); + } + + public static ShapeIndexGeography decode(UnsafeInput in, EncodeTag tag) throws IOException { + throw new IOException("Decode() not implemented for ShapeIndexGeography()"); Review Comment: The decode method for ShapeIndexGeography is stubbed out and will always throw. Implement the decoding logic or remove this unused stub to avoid runtime failures when decoding SHAPE_INDEX tagged data. ```suggestion // 1) Read the length-prefixed payload int payloadLength = in.readInt(/* optimizePositive= */ false); byte[] payload = new byte[payloadLength]; in.readBytes(payload); // 2) Decode the shapes from the payload Input tmpIn = new Input(payload); S2ShapeIndex shapeIndex = new S2ShapeIndex(); switch (tag.getCodingHint()) { case FAST: VectorCoder.FAST_SHAPE.decode(tmpIn, shapeIndex.getShapes()); break; case COMPACT: VectorCoder.COMPACT_SHAPE.decode(tmpIn, shapeIndex.getShapes()); break; default: throw new IOException("Unknown coding hint: " + tag.getCodingHint()); } // 3) Decode the quadtree structure S2ShapeIndexCoder.INSTANCE.decode(in, shapeIndex); // 4) Construct and return the ShapeIndexGeography object ShapeIndexGeography geography = new ShapeIndexGeography(); geography.shapeIndex = shapeIndex; return geography; ``` ########## common/src/main/java/org/apache/sedona/common/S2Geography/GeographyCollection.java: ########## @@ -0,0 +1,147 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sedona.common.S2Geography; + +import com.esotericsoftware.kryo.io.UnsafeInput; +import com.esotericsoftware.kryo.io.UnsafeOutput; +import com.google.common.collect.ImmutableList; +import com.google.common.geometry.*; +import java.io.*; +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; +import java.util.logging.Logger; + +/** A Geography wrapping zero or more Geography objects, representing a GEOMETRYCOLLECTION. */ +public class GeographyCollection extends S2Geography { + private static final Logger logger = Logger.getLogger(GeographyCollection.class.getName()); + + public final List<S2Geography> features; + public final List<Integer> numShapesList; + public int totalShapes; + + /** Constructs an empty GeographyCollection. */ + public GeographyCollection() { + super(GeographyKind.GEOGRAPHY_COLLECTION); + this.features = new ArrayList<>(); + this.numShapesList = new ArrayList<>(); + this.totalShapes = 0; + } + + /** Wraps existing Geography features. */ + public GeographyCollection(List<S2Geography> features) { + super(GeographyKind.GEOGRAPHY_COLLECTION); + this.features = new ArrayList<>(features); + this.numShapesList = new ArrayList<>(); + this.totalShapes = 0; + countShapes(); + } + + @Override + public int dimension() { + // Mixed or empty → return -1; uniform → return 0,1,2 + return computeDimensionFromShapes(); + } + + @Override + public int numShapes() { + return totalShapes; + } + + @Override + public S2Shape shape(int id) { + int sum = 0; + for (int i = 0; i < features.size(); i++) { + int n = numShapesList.get(i); + sum += n; + if (id < sum) { + // index within this feature + return features.get(i).shape(id - (sum - n)); + } + } + throw new IllegalArgumentException("Shape id out of bounds: " + id); + } + + @Override + public S2Region region() { + Collection<S2Region> regs = new ArrayList<>(); + for (S2Geography geo : features) { + regs.add(geo.region()); + } + return new S2RegionUnion(regs); + } + + /** Returns an immutable copy of the features list. */ + public List<S2Geography> getFeatures() { + return ImmutableList.copyOf(features); + } + + @Override + public void encode(UnsafeOutput out, EncodeOptions opts) throws IOException { + // Top-level collection encodes its size and then each child with tagging + // Never include coverings for children (only a top-level concept) + EncodeOptions child_options = opts; + child_options.setIncludeCovering(false); + out.writeInt(features.size()); + for (S2Geography feature : features) { + feature.encodeTagged(out, child_options); + } + out.flush(); + } + + /** Decodes a GeographyCollection from a tagged input stream. */ + public static GeographyCollection decode(UnsafeInput in, EncodeTag tag) throws IOException { + GeographyCollection geo = new GeographyCollection(); + + // Handle EMPTY flag + if ((tag.getFlags() & EncodeTag.FLAG_EMPTY) != 0) { + logger.fine("Decoded empty GeographyCollection."); + return geo; + } + + // Skip any covering data + tag.skipCovering(in); + + // 3) Ensure we have at least 4 bytes for the count + if (in.available() < Integer.BYTES) { + throw new IOException("PolygonGeography.decodeTagged error: insufficient header bytes"); Review Comment: The error message references PolygonGeography in GeographyCollection.decode. Update it to mention GeographyCollection.decodeTagged for clarity and correct context. ```suggestion throw new IOException("GeographyCollection.decodeTagged error: insufficient header bytes"); ``` ########## common/src/main/java/org/apache/sedona/common/S2Geography/GeographyIndex.java: ########## @@ -0,0 +1,115 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sedona.common.S2Geography; + +import com.google.common.geometry.S2CellId; +import com.google.common.geometry.S2Iterator; +import com.google.common.geometry.S2ShapeIndex; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Set; + +public class GeographyIndex { + private final S2ShapeIndex index; + private List<Integer> values; + + public GeographyIndex() { + this(new S2ShapeIndex.Options()); + } + + public GeographyIndex(S2ShapeIndex.Options options) { + this.index = new S2ShapeIndex(options); + this.values = new ArrayList<>(); // list of shape id + } + + public void add(S2Geography geog, int value) { + for (int i = 0; i < geog.numShapes(); i++) { + index.add(geog.shape(i)); + int shapeId = index.getShapes().size(); + values.add(shapeId, value); Review Comment: Using values.add(shapeId, value) can cause IndexOutOfBoundsException if the list hasn’t been pre-sized. Consider using values.add(value) or ensure the list is expanded to accommodate the index first. ```suggestion while (values.size() <= shapeId) { values.add(null); // Add placeholder elements to expand the list } values.set(shapeId, value); // Set the value at the correct index ``` ########## common/src/main/java/org/apache/sedona/common/S2Geography/EncodedShapeIndexGeography.java: ########## @@ -0,0 +1,140 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sedona.common.S2Geography; + +import com.esotericsoftware.kryo.io.Input; +import com.esotericsoftware.kryo.io.UnsafeInput; +import com.esotericsoftware.kryo.io.UnsafeOutput; +import com.google.common.geometry.*; +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.logging.Logger; + +public class EncodedShapeIndexGeography extends S2Geography { + private static final Logger logger = Logger.getLogger(EncodedShapeIndexGeography.class.getName()); + + public S2ShapeIndex shapeIndex; + + /** Build an empty ShapeIndexGeography. */ + public EncodedShapeIndexGeography() { + super(GeographyKind.ENCODED_SHAPE_INDEX); + this.shapeIndex = new S2ShapeIndex(); + } + + @Override + public int dimension() { + return -1; + } + + @Override + public int numShapes() { + return shapeIndex.getShapes().size(); + } + + @Override + public S2Shape shape(int id) { + return shapeIndex.getShapes().get(id); + } + + @Override + public S2Region region() { + return new S2ShapeIndexRegion(shapeIndex); + } + + /** + * Index every S2Shape from the given Geography. + * + * @return the last shapeId assigned. + */ + public int addIndex(S2Geography geog) { + int lastId = -1; + for (int i = 0, n = geog.numShapes(); i < n; i++) { + shapeIndex.add(geog.shape(i)); + // since add() appends to the end, its index is size-1 + lastId = shapeIndex.getShapes().size() - 1; + } + return lastId; + } + + /** Add one raw shape into the index, return its new ID */ + public int addIndex(S2Shape shape) { + shapeIndex.add(shape); + return shapeIndex.getShapes().size() - 1; + } + + @Override + protected void encode(UnsafeOutput os, EncodeOptions opts) throws IOException { + throw new IOException("Encode() not implemented for EncodedShapeIndexGeography()"); + } + // decode + /** This is what decodeTagged() actually calls */ + public static EncodedShapeIndexGeography decode(Input in, EncodeTag tag) throws IOException { + // cast to UnsafeInput—will work if you always pass a Kryo-backed stream + if (!(in instanceof UnsafeInput)) { + throw new IllegalArgumentException("Expected UnsafeInput"); + } + return decode((UnsafeInput) in, tag); + } + + public static EncodedShapeIndexGeography decode(UnsafeInput in, EncodeTag tag) + throws IOException { + EncodedShapeIndexGeography encodedShapeIndexGeography = new EncodedShapeIndexGeography(); + + // EMPTY + if ((tag.getFlags() & EncodeTag.FLAG_EMPTY) != 0) { + logger.fine("Decoded empty PolygonGeography."); Review Comment: The log statement in EncodedShapeIndexGeography.decode incorrectly mentions PolygonGeography. It should reference EncodedShapeIndexGeography to avoid confusion when reading logs. ```suggestion logger.fine("Decoded empty EncodedShapeIndexGeography."); ``` -- 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]
