You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "jiayuasu (via GitHub)" <gi...@apache.org> on 2026/02/08 09:12:57 UTC
[PR] [GH-908] Add ST_GeoHashNeighbors and ST_GeoHashNeighbor functions [sedona]
jiayuasu opened a new pull request, #2628:
URL: https://github.com/apache/sedona/pull/2628
## What changes were proposed in this pull request?
Implements #908: Adds two geohash neighbor functions across all platforms:
- **ST_GeoHashNeighbors(geohash)**: Returns an array of 8 neighboring geohash cells (N, NE, E, SE, S, SW, W, NW)
- **ST_GeoHashNeighbor(geohash, direction)**: Returns a single neighbor in the given direction (n, ne, e, se, s, sw, w, nw, case-insensitive)
Algorithm ported from geohash-java (Apache 2.0 license).
### Changes
- **GeoHashUtils.java** (new): Shared base32 constants and decode lookup, eliminating duplication across GeoHashDecoder, PointGeoHashEncoder, and GeoHashNeighbor
- **GeoHashNeighbor.java** (new): Core neighbor computation using bit-interleaving approach
- **GeoHashDecoder.java**, **PointGeoHashEncoder.java**: Refactored to use shared GeoHashUtils constants
- **InferredExpression.scala**: Added Array[String] type support for Spark SQL
- **Spark**: Registered in Catalog, Functions, st_functions
- **Flink**: Registered in Catalog, Functions with @DataTypeHint
- **Snowflake**: Registered in UDFs and UDFsV2
- **Python**: Added to st_functions.py
- **Tests**: Common unit tests (9), Spark SQL tests, Flink tests, Snowflake tests
- **Docs**: Spark, Flink, Snowflake API documentation
## How was this patch tested?
- 9 common unit tests covering known values, all 8 neighbors, circular movement, case insensitivity, null/empty/invalid inputs
- Spark SQL functional tests
- Flink SQL functional tests
- Snowflake tester tests (TestFunctions + TestFunctionsV2)
- All modules compile successfully
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-908] Add ST_GeoHashNeighbors and ST_GeoHashNeighbor functions [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu merged PR #2628:
URL: https://github.com/apache/sedona/pull/2628
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-908] Add ST_GeoHashNeighbors and ST_GeoHashNeighbor functions [sedona]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2628:
URL: https://github.com/apache/sedona/pull/2628#discussion_r2781126589
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/InferredExpression.scala:
##########
@@ -220,6 +222,18 @@ object InferredTypes {
expr.asString(input)
} else if (t =:= typeOf[Array[Long]]) { expr => input =>
expr.eval(input).asInstanceOf[ArrayData].toLongArray()
+ } else if (t =:= typeOf[Array[String]]) { expr => input =>
+ expr.eval(input).asInstanceOf[ArrayData] match {
+ case null => null
+ case arrayData: ArrayData =>
+ (0 until arrayData.numElements()).map { i =>
+ if (arrayData.isNullAt(i)) null
+ else {
+ val utf8 = arrayData.getUTF8String(i)
+ if (utf8 == null) null else utf8.toString
+ }
+ }.toArray
Review Comment:
This conversion allocates an intermediate Scala collection via `(0 until ...).map(...).toArray`. Since this path can be hot for expression evaluation, consider switching to a pre-sized `new Array[String](numElements)` plus a `while` loop to reduce allocations and improve throughput.
```suggestion
val n = arrayData.numElements()
val result = new Array[String](n)
var i = 0
while (i < n) {
if (arrayData.isNullAt(i)) {
result(i) = null
} else {
val utf8 = arrayData.getUTF8String(i)
result(i) = if (utf8 == null) null else utf8.toString
}
i += 1
}
result
```
##########
common/src/main/java/org/apache/sedona/common/utils/GeoHashUtils.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.utils;
+
+import java.util.Arrays;
+
+/**
+ * Shared constants and utilities for geohash encoding/decoding.
+ *
+ * <p>Centralizes the base32 alphabet and decode lookup table previously duplicated across {@link
+ * GeoHashDecoder}, {@link PointGeoHashEncoder}, and {@link GeoHashNeighbor}.
+ */
+public class GeoHashUtils {
+
+ /** The base32 alphabet used by geohash encoding (Gustavo Niemeyer's specification). */
+ public static final String BASE32 = "0123456789bcdefghjkmnpqrstuvwxyz";
+
+ /** Base32 character array for index-based lookup. */
+ public static final char[] BASE32_CHARS = BASE32.toCharArray();
+
+ /** Bit masks for extracting 5-bit groups: {16, 8, 4, 2, 1}. */
+ public static final int[] BITS = new int[] {16, 8, 4, 2, 1};
+
+ /** Number of bits per base32 character. */
+ public static final int BITS_PER_CHAR = 5;
+
+ /**
+ * Reverse lookup array: maps a character (as int) to its base32 index. Invalid characters map to
+ * -1.
+ */
+ public static final int[] DECODE_ARRAY = new int['z' + 1];
Review Comment:
GeoHashUtils exposes mutable arrays (`BASE32_CHARS`, `BITS`, `DECODE_ARRAY`) as `public static final`. Since arrays are mutable, any code in the same JVM can modify them at runtime and silently corrupt geohash encoding/decoding/neighboring results. Consider making these arrays `private` and exposing only safe methods (e.g., `decodeChar`) and/or returning defensive copies (`clone()`) where array access is required.
##########
common/src/main/java/org/apache/sedona/common/Functions.java:
##########
@@ -746,6 +746,14 @@ public static String geohash(Geometry geometry, int precision) {
return GeometryGeoHashEncoder.calculate(geometry, precision);
}
+ public static String[] geohashNeighbors(String geohash) {
+ return GeoHashNeighbor.getNeighbors(geohash);
+ }
+
+ public static String geohashNeighbor(String geohash, String direction) {
+ return GeoHashNeighbor.getNeighbor(geohash, direction);
+ }
Review Comment:
These are new public `common` APIs but currently have no JavaDoc describing key behavioral details (e.g., neighbor ordering for `geohashNeighbors`, accepted direction values/case-sensitivity, and what inputs throw `IllegalArgumentException` vs return `null`). Adding JavaDoc here would make the contract clear for downstream consumers beyond SQL wrappers.
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/st_functions.scala:
##########
@@ -246,6 +246,16 @@ object st_functions {
def ST_GeoHash(geometry: String, precision: Int): Column =
wrapExpression[ST_GeoHash](geometry, precision)
+ def ST_GeoHashNeighbors(geohash: Column): Column =
+ wrapExpression[ST_GeoHashNeighbors](geohash)
+ def ST_GeoHashNeighbors(geohash: String): Column =
+ wrapExpression[ST_GeoHashNeighbors](geohash)
+
+ def ST_GeoHashNeighbor(geohash: Column, direction: Column): Column =
+ wrapExpression[ST_GeoHashNeighbor](geohash, direction)
Review Comment:
The Scala DataFrame API wrappers provide `(Column, Column)` and `(String, String)` overloads for `ST_GeoHashNeighbor`, but not mixed overloads like `(Column, String)` / `(String, Column)`. Many Sedona Spark wrappers include these mixed overloads to avoid forcing callers to wrap literals with `lit(...)`. Consider adding the mixed overloads for consistency and usability.
```suggestion
wrapExpression[ST_GeoHashNeighbor](geohash, direction)
def ST_GeoHashNeighbor(geohash: Column, direction: String): Column =
wrapExpression[ST_GeoHashNeighbor](geohash, direction)
def ST_GeoHashNeighbor(geohash: String, direction: Column): Column =
wrapExpression[ST_GeoHashNeighbor](geohash, direction)
```
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [GH-908] Add ST_GeoHashNeighbors and ST_GeoHashNeighbor functions [sedona]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2628:
URL: https://github.com/apache/sedona/pull/2628#discussion_r2779099746
##########
snowflake-tester/src/test/java/org/apache/sedona/snowflake/snowsql/TestFunctionsV2.java:
##########
@@ -451,6 +451,19 @@ public void test_ST_GeoHash() {
"u3r0p");
}
+ @Test
+ public void test_ST_GeoHashNeighbors() {
+ registerUDFV2("ST_GeoHashNeighbors", String.class);
+ verifySqlSingleRes("select sedona.ST_GeoHashNeighbor('u1pb', 'n')", "u1pc");
Review Comment:
This test registers `ST_GeoHashNeighbors` but executes `ST_GeoHashNeighbor`, so it doesn’t validate the neighbors UDF at all. Update the query/assertion to call `sedona.ST_GeoHashNeighbors(...)` and verify the returned array contents (or at least the expected length/value at known indices).
```suggestion
verifySqlSingleRes("select ARRAY_SIZE(sedona.ST_GeoHashNeighbors('u1pb'))", 8);
```
##########
common/src/main/java/org/apache/sedona/common/utils/GeoHashNeighbor.java:
##########
@@ -0,0 +1,266 @@
+/*
+ * 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.utils;
+
+/**
+ * Utility class for computing geohash neighbors.
+ *
+ * <p>The neighbor algorithm is ported from the geohash-java library by Silvio Heuberger (Apache 2.0
+ * License). It works by de-interleaving the geohash bits into separate latitude/longitude
+ * components, incrementing/decrementing the appropriate component, masking for wrap-around, and
+ * re-interleaving back to a geohash string.
+ *
+ * @see <a href="https://github.com/kungfoo/geohash-java">geohash-java</a>
+ */
+public class GeoHashNeighbor {
+
+ private static final int MAX_BIT_PRECISION = 64;
+ private static final long FIRST_BIT_FLAGGED = 0x8000000000000000L;
+
+ // Shared base32 constants from GeoHashUtils
+ private static final char[] base32 = GeoHashUtils.BASE32_CHARS;
+ private static final int[] decodeArray = GeoHashUtils.DECODE_ARRAY;
+
+ /**
+ * Returns all 8 neighbors of the given geohash in the order: [N, NE, E, SE, S, SW, W, NW].
+ *
+ * @param geohash the geohash string
+ * @return array of 8 neighbor geohash strings, or null if input is null
+ */
+ public static String[] getNeighbors(String geohash) {
+ if (geohash == null) {
+ return null;
+ }
+
+ long[] parsed = parseGeohash(geohash);
+ long bits = parsed[0];
+ int significantBits = (int) parsed[1];
+
+ // Ported from geohash-java: GeoHash.getAdjacent()
+ String northern = neighborLat(bits, significantBits, 1);
+ String eastern = neighborLon(bits, significantBits, 1);
+ String southern = neighborLat(bits, significantBits, -1);
+ String western = neighborLon(bits, significantBits, -1);
+
+ // Diagonal neighbors: compose two cardinal moves
+ long[] northBits = parseGeohash(northern);
+ long[] southBits = parseGeohash(southern);
+
+ return new String[] {
+ northern,
+ neighborLon(northBits[0], significantBits, 1), // NE
+ eastern,
+ neighborLon(southBits[0], significantBits, 1), // SE
+ southern,
+ neighborLon(southBits[0], significantBits, -1), // SW
+ western,
+ neighborLon(northBits[0], significantBits, -1) // NW
+ };
+ }
+
+ /**
+ * Returns the neighbor of the given geohash in the specified direction.
+ *
+ * @param geohash the geohash string
+ * @param direction compass direction: "n", "ne", "e", "se", "s", "sw", "w", "nw"
+ * (case-insensitive)
+ * @return the neighbor geohash string, or null if input geohash is null
+ */
+ public static String getNeighbor(String geohash, String direction) {
+ if (geohash == null) {
+ return null;
+ }
+ if (direction == null) {
+ throw new IllegalArgumentException(
+ "Direction cannot be null. Valid values: n, ne, e, se, s, sw, w, nw");
+ }
+
+ long[] parsed = parseGeohash(geohash);
+ long bits = parsed[0];
+ int significantBits = (int) parsed[1];
+
+ switch (direction.toLowerCase()) {
+ case "n":
+ return neighborLat(bits, significantBits, 1);
+ case "s":
+ return neighborLat(bits, significantBits, -1);
+ case "e":
+ return neighborLon(bits, significantBits, 1);
+ case "w":
+ return neighborLon(bits, significantBits, -1);
+ case "ne":
+ {
+ long[] n = parseGeohash(neighborLat(bits, significantBits, 1));
+ return neighborLon(n[0], significantBits, 1);
+ }
+ case "se":
+ {
+ long[] s = parseGeohash(neighborLat(bits, significantBits, -1));
+ return neighborLon(s[0], significantBits, 1);
+ }
+ case "sw":
+ {
+ long[] s = parseGeohash(neighborLat(bits, significantBits, -1));
+ return neighborLon(s[0], significantBits, -1);
+ }
+ case "nw":
+ {
+ long[] n = parseGeohash(neighborLat(bits, significantBits, 1));
+ return neighborLon(n[0], significantBits, -1);
+ }
+ default:
+ throw new IllegalArgumentException(
+ "Invalid direction: '" + direction + "'. Valid values: n, ne, e, se, s, sw, w, nw");
+ }
+ }
+
+ /**
+ * Parses a geohash string into [bits, significantBits]. Based on geohash-java
+ * GeoHash.fromGeohashString().
+ */
+ private static long[] parseGeohash(String geohash) {
+ if (geohash.isEmpty()) {
+ throw new IllegalArgumentException("Geohash string cannot be empty");
+ }
+ long bits = 0;
+ int significantBits = 0;
+ for (int i = 0; i < geohash.length(); i++) {
+ char c = geohash.charAt(i);
+ int cd;
+ if (c >= decodeArray.length || (cd = decodeArray[c]) < 0) {
+ throw new IllegalArgumentException(
+ "Invalid character '" + c + "' in geohash '" + geohash + "'");
+ }
+ for (int j = 0; j < GeoHashUtils.BITS_PER_CHAR; j++) {
+ significantBits++;
+ bits <<= 1;
+ if ((cd & (16 >> j)) != 0) {
+ bits |= 0x1;
+ }
+ }
+ }
+ bits <<= (MAX_BIT_PRECISION - significantBits);
+ return new long[] {bits, significantBits};
+ }
Review Comment:
`parseGeohash` does not guard against inputs whose bit-length exceeds 64 (e.g., geohash length >= 13 => significantBits >= 65). This makes `bits <<= (MAX_BIT_PRECISION - significantBits)` a negative shift (masked by the JVM) and yields incorrect results. Add validation to reject geohashes where `geohash.length() * 5 > MAX_BIT_PRECISION` (or `significantBits > MAX_BIT_PRECISION`) with a clear `IllegalArgumentException`.
##########
spark/common/src/test/scala/org/apache/sedona/sql/functionTestScala.scala:
##########
@@ -4277,4 +4281,33 @@ class functionTestScala
FileUtils.deleteDirectory(new File(tmpDir))
}
+
+ it("Should pass ST_GeoHashNeighbors") {
+ var result = sparkSession.sql("SELECT ST_GeoHashNeighbors('u1pb')").first().getList[String](0)
+ assert(result.size() == 8)
+ assert(result.get(0) == "u1pc") // N
+ assert(result.get(2) == "u300") // E
+ assert(result.get(4) == "u0zz") // S
+ assert(result.get(6) == "u1p8") // W
+
+ result = sparkSession.sql("SELECT ST_GeoHashNeighbors('dqcjqc')").first().getList[String](0)
+ assert(result.size() == 8)
+ val expected =
+ Set("dqcjqf", "dqcjr4", "dqcjr1", "dqcjr0", "dqcjq9", "dqcjq8", "dqcjqb", "dqcjqd")
+ val actual = (0 until 8).map(result.get).toSet
+ assert(actual == expected)
+ }
+
+ it("Should pass ST_GeoHashNeighbor") {
+ var result = sparkSession.sql("SELECT ST_GeoHashNeighbor('u1pb', 'n')").first().getString(0)
+ assert(result == "u1pc")
+ result = sparkSession.sql("SELECT ST_GeoHashNeighbor('u1pb', 'e')").first().getString(0)
+ assert(result == "u300")
+ result = sparkSession.sql("SELECT ST_GeoHashNeighbor('u1pb', 's')").first().getString(0)
+ assert(result == "u0zz")
+ result = sparkSession.sql("SELECT ST_GeoHashNeighbor('u1pb', 'w')").first().getString(0)
+ assert(result == "u1p8")
+ result = sparkSession.sql("SELECT ST_GeoHashNeighbor('u1pb', 'NE')").first().getString(0)
+ assert(result == "u301")
Review Comment:
The expected `NE` neighbor value here is inconsistent with the docs example added in this PR (which shows `u1pd` as the NE neighbor in the ordered neighbors array) and with the Flink test expecting `u1pd` for `NE`. Align the expected values across Spark/Flink tests and the docs to the actual implementation output, so cross-engine behavior and documentation stay consistent.
```suggestion
assert(result == "u1pd")
```
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/InferredExpression.scala:
##########
@@ -263,6 +271,12 @@ object InferredTypes {
} else {
null
}
+ } else if (t =:= typeOf[Array[String]]) { output =>
+ if (output != null) {
+ ArrayData.toArrayData(output.asInstanceOf[Array[String]].map(UTF8String.fromString))
Review Comment:
This serialization path will throw if the `Array[String]` contains null elements (`UTF8String.fromString(null)` will NPE). Map nulls through (e.g., map to `null` UTF8String entries) before calling `ArrayData.toArrayData`.
```suggestion
ArrayData.toArrayData(
output.asInstanceOf[Array[String]].map { s =>
if (s != null) UTF8String.fromString(s) else null
})
```
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/InferredExpression.scala:
##########
@@ -220,6 +222,12 @@ object InferredTypes {
expr.asString(input)
} else if (t =:= typeOf[Array[Long]]) { expr => input =>
expr.eval(input).asInstanceOf[ArrayData].toLongArray()
+ } else if (t =:= typeOf[Array[String]]) { expr => input =>
+ expr.eval(input).asInstanceOf[ArrayData] match {
+ case null => null
+ case arrayData: ArrayData =>
+ (0 until arrayData.numElements()).map(i => arrayData.getUTF8String(i).toString).toArray
Review Comment:
This conversion will throw if the Spark `ArrayData` contains null elements (`getUTF8String(i)` can return null / `isNullAt(i)` true). Handle null slots by checking `arrayData.isNullAt(i)` and emitting `null` in the resulting `Array[String]` for those positions.
```suggestion
(0 until arrayData.numElements()).map { i =>
if (arrayData.isNullAt(i)) {
null
} else {
val utf8 = arrayData.getUTF8String(i)
if (utf8 == null) null else utf8.toString
}
}.toArray
```
##########
snowflake-tester/src/test/java/org/apache/sedona/snowflake/snowsql/TestFunctions.java:
##########
@@ -471,6 +471,19 @@ public void test_ST_GeoHash() {
"u3r0p");
}
+ @Test
+ public void test_ST_GeoHashNeighbors() {
+ registerUDF("ST_GeoHashNeighbors", String.class);
+ verifySqlSingleRes("select sedona.ST_GeoHashNeighbor('u1pb', 'n')", "u1pc");
Review Comment:
Same issue as V2: the test registers `ST_GeoHashNeighbors` but calls `ST_GeoHashNeighbor`, so the neighbors function is untested. Change the SQL to `sedona.ST_GeoHashNeighbors('u1pb')` and assert the expected array result.
```suggestion
verifySqlSingleRes("select ARRAY_SIZE(sedona.ST_GeoHashNeighbors('u1pb'))", 8);
```
--
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: issues-unsubscribe@sedona.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org