Kontinuation commented on code in PR #1992: URL: https://github.com/apache/sedona/pull/1992#discussion_r2176293405
########## common/src/main/java/org/apache/sedona/common/S2Geography/PolygonGeography.java: ########## @@ -0,0 +1,118 @@ +/* + * 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.List; +import java.util.logging.Logger; + +public class PolygonGeography extends S2Geography { + private static final Logger logger = Logger.getLogger(PolygonGeography.class.getName()); + + public final S2Polygon polygon; + + public PolygonGeography() { + super(GeographyKind.POLYGON); + this.polygon = new S2Polygon(); + } + + public PolygonGeography(S2Polygon polygon) { + super(GeographyKind.POLYGON); + this.polygon = polygon; + } + + @Override + public int dimension() { + return polygon == null ? -1 : 2; + } + + @Override + public int numShapes() { + return polygon == null ? 0 : 1; + } + + @Override + public S2Shape shape(int id) { + assert polygon != null; + return polygon.shape(); + } Review Comment: `this.polygon` should never be null (but can be empty). If we replicate the C++ implementation: * `dimension` should always return 2. * `numShapes` should be `return polygon.isEmpty()? 0: 1;`. ########## common/src/main/java/org/apache/sedona/common/S2Geography/PolygonGeography.java: ########## @@ -0,0 +1,118 @@ +/* + * 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.List; +import java.util.logging.Logger; + +public class PolygonGeography extends S2Geography { + private static final Logger logger = Logger.getLogger(PolygonGeography.class.getName()); + + public final S2Polygon polygon; + + public PolygonGeography() { + super(GeographyKind.POLYGON); + this.polygon = new S2Polygon(); + } + + public PolygonGeography(S2Polygon polygon) { + super(GeographyKind.POLYGON); + this.polygon = polygon; + } + + @Override + public int dimension() { + return polygon == null ? -1 : 2; + } + + @Override + public int numShapes() { + return polygon == null ? 0 : 1; + } + + @Override + public S2Shape shape(int id) { + assert polygon != null; + return polygon.shape(); + } + + @Override + public S2Region region() { + return this.polygon; + } + + @Override + public void getCellUnionBound(List<S2CellId> cellIds) { + super.getCellUnionBound(cellIds); + } + + @Override + public void encode(UnsafeOutput out, EncodeOptions opts) throws IOException { + // 3) Write number of polygon + out.writeInt(numShapes()); + // 4) Encode polygon Review Comment: There's no need to write the number of polygon, since the polygon will not be encoded when it is empty (handled by `encodeTagged` in S2Geography). When we reach here, there must be only 1 polygon to be encoded. ########## common/src/main/java/org/apache/sedona/common/S2Geography/PolygonGeography.java: ########## @@ -0,0 +1,118 @@ +/* + * 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.List; +import java.util.logging.Logger; + +public class PolygonGeography extends S2Geography { + private static final Logger logger = Logger.getLogger(PolygonGeography.class.getName()); + + public final S2Polygon polygon; + + public PolygonGeography() { + super(GeographyKind.POLYGON); + this.polygon = new S2Polygon(); + } + + public PolygonGeography(S2Polygon polygon) { + super(GeographyKind.POLYGON); + this.polygon = polygon; + } + + @Override + public int dimension() { + return polygon == null ? -1 : 2; + } + + @Override + public int numShapes() { + return polygon == null ? 0 : 1; + } + + @Override + public S2Shape shape(int id) { + assert polygon != null; + return polygon.shape(); + } + + @Override + public S2Region region() { + return this.polygon; + } + + @Override + public void getCellUnionBound(List<S2CellId> cellIds) { + super.getCellUnionBound(cellIds); + } + + @Override + public void encode(UnsafeOutput out, EncodeOptions opts) throws IOException { + // 3) Write number of polygon + out.writeInt(numShapes()); + // 4) Encode polygon + polygon.encode(out); + out.flush(); + } + + /** This is what decodeTagged() actually calls */ + public static PolygonGeography 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 PolygonGeography decode(UnsafeInput in, EncodeTag tag) throws IOException { + PolygonGeography geo = new PolygonGeography(); + + // EMPTY + if ((tag.getFlags() & EncodeTag.FLAG_EMPTY) != 0) { + logger.fine("Decoded empty PolygonGeography."); + return geo; + } + + // 2) Skip past any covering cell-IDs written by encodeTagged + 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"); + } + + // 4) Read the number of polylines (4-byte) + int count = in.readInt(); + // Decode each polygon + if (count != 1) { + throw new IOException( + "PolygonGeography.decode error: expected exactly one polygon, but found " + count); + } Review Comment: These steps are not needed as well. -- 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]
