You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@sedona.apache.org by "james-willis (via GitHub)" <gi...@apache.org> on 2025/07/09 02:12:01 UTC
[PR] [GH-2074] Add libpostal integration [sedona]
james-willis opened a new pull request, #2077:
URL: https://github.com/apache/sedona/pull/2077
## Did you read the Contributor Guide?
- Yes, I have read the [Contributor Rules](https://sedona.apache.org/latest/community/rule/) and [Contributor Development Guide](https://sedona.apache.org/latest/community/develop/)
## Is this PR related to a ticket?
- Yes, and the PR name follows the format `[GH-2074] my subject`. Closes [#2074](https://github.com/apache/sedona/issues/2074)
## What changes were proposed in this PR?
This add ExpandAddress and ParseAddress functions backed by libpostal, including support for code gen.
## How was this patch tested?
Unit Tests
## Did this PR include necessary documentation updates?
TODO
~- Yes, I have updated the documentation.~
--
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-2074] Add libpostal integration [sedona]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2205841105
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Functions.scala:
##########
@@ -1831,3 +1835,140 @@ case class ST_InterpolatePoint(inputExpressions: Seq[Expression])
protected def withNewChildrenInternal(newChildren: IndexedSeq[Expression]) =
copy(inputExpressions = newChildren)
}
+
+case class ExpandAddress(address: Expression)
+ extends UnaryExpression
+ with ImplicitCastInputTypes
+ with CodegenFallback
+ with FoldableExpression
+ with Serializable {
+
+ def this(children: Seq[Expression]) = this(children.head)
+
+ lazy private final val expander = {
+ val conf = SedonaConf.fromSparkEnv
+ getExpanderFromConf(conf.getLibPostalDataDir, conf.getLibPostalUseSenzing)
+ }
+
+ override def nullable: Boolean = true
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ override def dataType: DataType = ArrayType(StringType)
+
+ override def eval(input: InternalRow): Any = {
+ val addressVal = address.eval(input)
+ if (addressVal == null) {
+ null
+ } else {
+ new GenericArrayData(
+ expander
+ .expandAddress(addressVal.asInstanceOf[UTF8String].toString)
+ .map(UTF8String.fromString))
+ }
+ }
+
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ val expanderRef = ctx.addReferenceObj("expander", expander, classOf[AddressExpander].getName)
+ val utf8StringClass = "org.apache.spark.unsafe.types.UTF8String"
+ val arrayDataClass = "org.apache.spark.sql.catalyst.util.GenericArrayData"
+ val addressRef = child.genCode(ctx)
+ val address = addressRef.value
+ ev.copy(code = code"""
+ ${addressRef.code}
+ boolean ${ev.isNull} = ${addressRef.isNull};
+ $arrayDataClass ${ev.value} = null;
+
+ if (!${ev.isNull}) {
+ String[] expandedAddressArray = $expanderRef.expandAddress($address.toString());
+ $utf8StringClass[] utf8Strings = new $utf8StringClass[expandedAddressArray.length];
+ for (int j = 0; j < expandedAddressArray.length; j++) {
+ utf8Strings[j] = $utf8StringClass.fromString(expandedAddressArray[j]);
+ }
+ ${ev.value} = new $arrayDataClass(utf8Strings);
+ } else {
+ ${ev.value} = new $arrayDataClass(new $utf8StringClass[0]);
Review Comment:
practically this isnt true because of `boolean ${ev.isNull} = ${addressRef.isNull};`. I will update the code anyway
--
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-2074] Add libpostal integration [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2201910327
##########
pom.xml:
##########
@@ -422,6 +422,18 @@
<id>Spark Packages</id>
<url>https://repos.spark-packages.org/</url>
</repository>
+<!-- TODO remove me and use jpostal actual release-->
+ <repository>
Review Comment:
Do we still need this?
--
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-2074] Add libpostal integration [sedona]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2205769792
##########
spark/common/src/test/scala/org/apache/sedona/sql/GeoStatsSuite.scala:
##########
@@ -29,6 +29,10 @@ import org.apache.spark.sql.sedona_sql.expressions.st_functions.{ST_DBSCAN, ST_L
class GeoStatsSuite extends TestBaseScala {
private val spark = sparkSession
Review Comment:
[nitpick] This override disables adaptive query execution for all tests in GeoStatsSuite, which may introduce inconsistency with other test suites; consider adding a comment explaining why it's needed or scoping it to only the tests that require it.
```suggestion
// Disabling adaptive query execution globally for this suite to ensure deterministic query plans.
// If specific tests require this configuration, consider scoping it to those tests instead.
```
##########
spark/common/src/test/java/org/apache/sedona/core/utils/SedonaConfTest.java:
##########
@@ -57,5 +57,7 @@ public void testBytesFromString() {
assertEquals(-1, SedonaConf.bytesFromString("-1"));
assertEquals(1024, SedonaConf.bytesFromString("1k"));
assertEquals(2097152, SedonaConf.bytesFromString("2MB"));
+ // fromSparkEnv means we dont have access to default values so sometimes we get null as input
Review Comment:
Typographical error: 'dont' should be written as 'don't' for clarity.
```suggestion
// fromSparkEnv means we don't have access to default values so sometimes we get null as input
```
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Functions.scala:
##########
@@ -22,19 +22,23 @@ import org.apache.sedona.common.geometryObjects.Geography
import org.apache.sedona.common.{Functions, FunctionsGeoTools}
import org.apache.sedona.common.sphere.{Haversine, Spheroid}
import org.apache.sedona.common.utils.{InscribedCircle, ValidDetail}
+import org.apache.sedona.core.utils.SedonaConf
import org.apache.sedona.sql.utils.GeometrySerializer
import org.apache.spark.sql.catalyst.InternalRow
-import org.apache.spark.sql.catalyst.expressions.codegen.CodegenFallback
-import org.apache.spark.sql.catalyst.expressions.{ExpectsInputTypes, Expression, Generator, Nondeterministic}
-import org.apache.spark.sql.catalyst.util.ArrayData
+import org.apache.spark.sql.catalyst.expressions.codegen.{CodegenContext, CodegenFallback, ExprCode}
+import org.apache.spark.sql.catalyst.expressions.{ExpectsInputTypes, Expression, Generator, ImplicitCastInputTypes, Nondeterministic, UnaryExpression}
+import org.apache.spark.sql.catalyst.util.{ArrayData, GenericArrayData}
import org.apache.spark.sql.sedona_sql.UDT.GeometryUDT
import org.apache.spark.sql.sedona_sql.expressions.implicits._
import org.apache.spark.sql.types._
import org.locationtech.jts.algorithm.MinimumBoundingCircle
import org.locationtech.jts.geom._
import org.apache.spark.sql.sedona_sql.expressions.InferrableFunctionConverter._
+import org.apache.spark.sql.sedona_sql.expressions.LibPostalUtils.{getExpanderFromConf, getParserFromConf}
import org.apache.spark.unsafe.types.UTF8String
import org.apache.spark.util.Utils
+import com.mapzen.jpostal.{AddressExpander, AddressParser}
+import org.apache.spark.sql.catalyst.expressions.codegen.Block.BlockHelper
Review Comment:
Imported BlockHelper is not used in this file; consider removing this unused import to keep the code clean.
```suggestion
```
##########
spark/common/src/test/scala/org/apache/sedona/sql/AddressProcessingFunctionsTest.scala:
##########
@@ -0,0 +1,249 @@
+/*
+ * 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.sql
+
+import org.apache.sedona.core.utils.SedonaConf
+import org.apache.spark.sql.sedona_sql.expressions.st_functions.{ExpandAddress, ParseAddress}
+import org.apache.spark.sql.{Row, functions => f}
+import org.scalatest.BeforeAndAfterEach
+import org.slf4j.LoggerFactory
+
+import java.nio.file.{Files, Paths}
+import scala.collection.mutable
+
+class AddressProcessingFunctionsTest extends TestBaseScala with BeforeAndAfterEach {
+ private val logger = LoggerFactory.getLogger(getClass)
+ var clearedLibPostal = false
+
+ def clearLibpostalDataDir(): String = {
+ val dir = SedonaConf.fromActiveSession().getLibPostalDataDir
+ if (dir != null && dir.nonEmpty) {
+ try {
+ Files
+ .walk(Paths.get(dir))
+ .sorted(java.util.Comparator.reverseOrder())
+ .forEach(path => Files.deleteIfExists(path))
+ } catch {
+ case e: Exception =>
+ logger.warn(s"Failed to clear libpostal data directory: ${e.getMessage}")
+ }
+ }
+ dir
+ }
+
+ override def beforeEach(): Unit = {
+ super.beforeEach()
+ if (!clearedLibPostal) {
+ clearedLibPostal = true
+ clearLibpostalDataDir()
+ }
+ }
+
+ describe("ExpandAddress") {
+ it("should return expected normalized forms") {
+ val resultDf = sparkSession
+ .sql(
+ "SELECT ExpandAddress('781 Franklin Ave Crown Heights Brooklyn NY 11216 USA') as normalized")
+ resultDf.write
+ .format("noop")
+ .mode("overwrite")
+ .save() // Tests a deserialization step that collect does not
+ val result = resultDf.collect
+ .take(1)(0)(0)
+ .asInstanceOf[mutable.Seq[String]]
+ assert(result.contains("781 franklin avenue crown heights brooklyn new york 11216 usa"))
+ }
+
+ it("should return null for null input") {
+ val result = sparkSession.sql("SELECT ExpandAddress(NULL) as normalized").collect()
+ assert(result.head.get(0) == null)
+ }
+
+ it("should should work when chained with explode") {
+ val result = sparkSession
+ .sql("SELECT Explode(ExpandAddress('781 Franklin Ave Crown Heights Brooklyn NY 11216 USA')) as normalized")
+ .collect()
+ .map(_.getString(0))
+ assert(result.contains("781 franklin avenue crown heights brooklyn new york 11216 usa"))
+ }
+ }
+
+ describe("ParseAddress") {
+ it("should return expected label/value pairs") {
+ val resultDf = sparkSession
+ .sql(
+ "SELECT ParseAddress('781 Franklin Ave Crown Heights Brooklyn NY 11216 USA') as parsed")
+ resultDf.write
+ .format("noop")
+ .mode("overwrite")
+ .save() // Tests a deserialization step that collect does not
+ val result = resultDf
+ .take(1)(0)(0)
+ .asInstanceOf[mutable.Seq[Row]]
+ .map(row => row.getAs[String]("label") -> row.getAs[String]("value"))
+ .toMap
+
+ assert(result.contains("road"))
+ assert(result("road") == "franklin ave")
+ assert(result.contains("city_district"))
+ assert(result("city_district") == "brooklyn")
+ assert(result.contains("house_number"))
+ assert(result("house_number") == "781")
+ assert(result.contains("postcode"))
+ assert(result("postcode") == "11216")
+ assert(result.contains("country"))
+ assert(result("country").toLowerCase.contains("usa"))
+ assert(result.contains("suburb"))
+ assert(result("suburb").toLowerCase.contains("crown heights"))
+ }
+
+ it("should return null for null input") {
+ val result = sparkSession.sql("SELECT ParseAddress(NULL) as parsed").collect()
+ assert(result.head.get(0) == null)
+ }
+
+ it("should should work when chained with explode") {
Review Comment:
Duplicate 'should' in the test description; it should read 'should work when chained with explode'.
```suggestion
it("should work when chained with explode") {
```
##########
spark/common/src/test/scala/org/apache/sedona/sql/AddressProcessingFunctionsTest.scala:
##########
@@ -0,0 +1,249 @@
+/*
+ * 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.sql
+
+import org.apache.sedona.core.utils.SedonaConf
+import org.apache.spark.sql.sedona_sql.expressions.st_functions.{ExpandAddress, ParseAddress}
+import org.apache.spark.sql.{Row, functions => f}
+import org.scalatest.BeforeAndAfterEach
+import org.slf4j.LoggerFactory
+
+import java.nio.file.{Files, Paths}
+import scala.collection.mutable
+
+class AddressProcessingFunctionsTest extends TestBaseScala with BeforeAndAfterEach {
+ private val logger = LoggerFactory.getLogger(getClass)
+ var clearedLibPostal = false
+
+ def clearLibpostalDataDir(): String = {
+ val dir = SedonaConf.fromActiveSession().getLibPostalDataDir
+ if (dir != null && dir.nonEmpty) {
+ try {
+ Files
+ .walk(Paths.get(dir))
+ .sorted(java.util.Comparator.reverseOrder())
+ .forEach(path => Files.deleteIfExists(path))
+ } catch {
+ case e: Exception =>
+ logger.warn(s"Failed to clear libpostal data directory: ${e.getMessage}")
+ }
+ }
+ dir
+ }
+
+ override def beforeEach(): Unit = {
+ super.beforeEach()
+ if (!clearedLibPostal) {
+ clearedLibPostal = true
+ clearLibpostalDataDir()
+ }
+ }
+
+ describe("ExpandAddress") {
+ it("should return expected normalized forms") {
+ val resultDf = sparkSession
+ .sql(
+ "SELECT ExpandAddress('781 Franklin Ave Crown Heights Brooklyn NY 11216 USA') as normalized")
+ resultDf.write
+ .format("noop")
+ .mode("overwrite")
+ .save() // Tests a deserialization step that collect does not
+ val result = resultDf.collect
+ .take(1)(0)(0)
+ .asInstanceOf[mutable.Seq[String]]
+ assert(result.contains("781 franklin avenue crown heights brooklyn new york 11216 usa"))
+ }
+
+ it("should return null for null input") {
+ val result = sparkSession.sql("SELECT ExpandAddress(NULL) as normalized").collect()
+ assert(result.head.get(0) == null)
+ }
+
+ it("should should work when chained with explode") {
Review Comment:
Duplicate 'should' in the test description; it should read 'should work when chained with explode'.
```suggestion
it("should work when chained with explode") {
```
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Functions.scala:
##########
@@ -1831,3 +1835,140 @@ case class ST_InterpolatePoint(inputExpressions: Seq[Expression])
protected def withNewChildrenInternal(newChildren: IndexedSeq[Expression]) =
copy(inputExpressions = newChildren)
}
+
+case class ExpandAddress(address: Expression)
+ extends UnaryExpression
+ with ImplicitCastInputTypes
+ with CodegenFallback
+ with FoldableExpression
+ with Serializable {
+
+ def this(children: Seq[Expression]) = this(children.head)
+
+ lazy private final val expander = {
+ val conf = SedonaConf.fromSparkEnv
+ getExpanderFromConf(conf.getLibPostalDataDir, conf.getLibPostalUseSenzing)
+ }
+
+ override def nullable: Boolean = true
+
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+
+ override def dataType: DataType = ArrayType(StringType)
+
+ override def eval(input: InternalRow): Any = {
+ val addressVal = address.eval(input)
+ if (addressVal == null) {
+ null
+ } else {
+ new GenericArrayData(
+ expander
+ .expandAddress(addressVal.asInstanceOf[UTF8String].toString)
+ .map(UTF8String.fromString))
+ }
+ }
+
+ override protected def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+ val expanderRef = ctx.addReferenceObj("expander", expander, classOf[AddressExpander].getName)
+ val utf8StringClass = "org.apache.spark.unsafe.types.UTF8String"
+ val arrayDataClass = "org.apache.spark.sql.catalyst.util.GenericArrayData"
+ val addressRef = child.genCode(ctx)
+ val address = addressRef.value
+ ev.copy(code = code"""
+ ${addressRef.code}
+ boolean ${ev.isNull} = ${addressRef.isNull};
+ $arrayDataClass ${ev.value} = null;
+
+ if (!${ev.isNull}) {
+ String[] expandedAddressArray = $expanderRef.expandAddress($address.toString());
+ $utf8StringClass[] utf8Strings = new $utf8StringClass[expandedAddressArray.length];
+ for (int j = 0; j < expandedAddressArray.length; j++) {
+ utf8Strings[j] = $utf8StringClass.fromString(expandedAddressArray[j]);
+ }
+ ${ev.value} = new $arrayDataClass(utf8Strings);
+ } else {
+ ${ev.value} = new $arrayDataClass(new $utf8StringClass[0]);
Review Comment:
In doGenCode the else branch returns an empty array for null inputs, but eval returns null; this inconsistent null-handling can lead to incorrect behavior under codegen. Update the generated code to set ev.isNull and return null when the input is null.
##########
python/sedona/spark/sql/st_functions.py:
##########
@@ -32,6 +32,25 @@
_call_st_function = partial(call_sedona_function, "st_functions")
Review Comment:
[nitpick] Missing @validate_argument_types decorator on ExpandAddress for input validation consistency; consider adding it as is done for ParseAddress.
```suggestion
@validate_argument_types
```
--
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-2074] Add libpostal integration [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2201909482
##########
.github/workflows/java.yml:
##########
@@ -65,6 +65,9 @@ jobs:
- spark: 4.0.0
scala: 2.13.8
jdk: '17'
+ - spark: 3.5.4
Review Comment:
Why is this needed?
--
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-2074] Add libpostal integration [sedona]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on PR #2077:
URL: https://github.com/apache/sedona/pull/2077#issuecomment-3063273490
actually turned out to be really easy to support java 11: https://github.com/wherobots/jpostal/pull/13
--
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-2074] Add libpostal integration [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on PR #2077:
URL: https://github.com/apache/sedona/pull/2077#issuecomment-3062827095
@james-willis Since the jpostal under Wherobots is already a fork, why not just compile it against Java 11? What specific Java 16+ features were used in the JNI binding and can we drop it?
--
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-2074] Add libpostal integration [sedona]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2201915717
##########
python/tests/sql/test_dataframe_api.py:
##########
@@ -1783,3 +1783,29 @@ def test_lof(self):
df.withColumn(
"localOutlierFactor", ST_LocalOutlierFactor("geometry", 2, False)
).collect()
+
+ def test_expand_address_df_api(self):
+ df = (
+ self.spark.range(1)
+ .selectExpr(
+ "'781 Franklin Ave Crown Heights Brooklyn NY 11216 USA' as address"
+ )
+ .cache()
+ ) # cache to avoid Constant Folding Optimization
+ df = df.select(ExpandAddress("address").alias("normalized"))
+ # Actually running downloads the model and is very expensive, so we just check the plan
+ # Checking the plan should allow us to verify that the function is correctly registered
+ df.explain()
Review Comment:
Couple reasons I didnt do this:
1. AFAIK there is not a real opportunity to inspect the plan contents. The pyspark API doesn't expose anything to programmatically interact with the query plan: https://spark.apache.org/docs/latest/api/python/reference/pyspark.sql/dataframe.html
2. I didn't think it was particularly valuable. If the explain doesn't throw that implies that the function was found on the path, which IMO is the primary purpose of this test.
--
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-2074] Add libpostal integration [sedona]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2193915104
##########
python/sedona/spark/sql/st_functions.py:
##########
@@ -20,7 +20,7 @@
from typing import Optional, Union
from pyspark.sql import Column
-from pyspark.sql.functions import lit
+from pyspark.sql.functions import lit, col
Review Comment:
The `lit` import is not used in this module. Removing unused imports helps keep the code clean.
```suggestion
from pyspark.sql.functions import col
```
--
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-2074] Add libpostal integration [sedona]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2201910882
##########
.github/workflows/java.yml:
##########
@@ -65,6 +65,9 @@ jobs:
- spark: 4.0.0
scala: 2.13.8
jdk: '17'
+ - spark: 3.5.4
Review Comment:
since i fixed the jpostal java 17 thing it isnt strictly. I left it because It matches AWS Glue 5.0 and I think thats a good piece of test coverage.
##########
pom.xml:
##########
@@ -422,6 +422,18 @@
<id>Spark Packages</id>
<url>https://repos.spark-packages.org/</url>
</repository>
+<!-- TODO remove me and use jpostal actual release-->
+ <repository>
Review Comment:
yesh only just now did I finish publishing the release version
--
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-2074] Add libpostal integration [sedona]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2201911070
##########
pom.xml:
##########
@@ -422,6 +422,18 @@
<id>Spark Packages</id>
<url>https://repos.spark-packages.org/</url>
</repository>
+<!-- TODO remove me and use jpostal actual release-->
+ <repository>
Review Comment:
yes only just now did I finish publishing the release version
--
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-2074] Add libpostal integration [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu merged PR #2077:
URL: https://github.com/apache/sedona/pull/2077
--
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-2074] Add libpostal integration [sedona]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2205773047
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Functions.scala:
##########
@@ -22,19 +22,23 @@ import org.apache.sedona.common.geometryObjects.Geography
import org.apache.sedona.common.{Functions, FunctionsGeoTools}
import org.apache.sedona.common.sphere.{Haversine, Spheroid}
import org.apache.sedona.common.utils.{InscribedCircle, ValidDetail}
+import org.apache.sedona.core.utils.SedonaConf
import org.apache.sedona.sql.utils.GeometrySerializer
import org.apache.spark.sql.catalyst.InternalRow
-import org.apache.spark.sql.catalyst.expressions.codegen.CodegenFallback
-import org.apache.spark.sql.catalyst.expressions.{ExpectsInputTypes, Expression, Generator, Nondeterministic}
-import org.apache.spark.sql.catalyst.util.ArrayData
+import org.apache.spark.sql.catalyst.expressions.codegen.{CodegenContext, CodegenFallback, ExprCode}
+import org.apache.spark.sql.catalyst.expressions.{ExpectsInputTypes, Expression, Generator, ImplicitCastInputTypes, Nondeterministic, UnaryExpression}
+import org.apache.spark.sql.catalyst.util.{ArrayData, GenericArrayData}
import org.apache.spark.sql.sedona_sql.UDT.GeometryUDT
import org.apache.spark.sql.sedona_sql.expressions.implicits._
import org.apache.spark.sql.types._
import org.locationtech.jts.algorithm.MinimumBoundingCircle
import org.locationtech.jts.geom._
import org.apache.spark.sql.sedona_sql.expressions.InferrableFunctionConverter._
+import org.apache.spark.sql.sedona_sql.expressions.LibPostalUtils.{getExpanderFromConf, getParserFromConf}
import org.apache.spark.unsafe.types.UTF8String
import org.apache.spark.util.Utils
+import com.mapzen.jpostal.{AddressExpander, AddressParser}
+import org.apache.spark.sql.catalyst.expressions.codegen.Block.BlockHelper
Review Comment:
It is needed for the code gen in the libpostal functions
--
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-2074] Add libpostal integration [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2205636719
##########
python/tests/sql/test_dataframe_api.py:
##########
@@ -1783,3 +1783,29 @@ def test_lof(self):
df.withColumn(
"localOutlierFactor", ST_LocalOutlierFactor("geometry", 2, False)
).collect()
+
+ def test_expand_address_df_api(self):
+ df = (
+ self.spark.range(1)
+ .selectExpr(
+ "'781 Franklin Ave Crown Heights Brooklyn NY 11216 USA' as address"
+ )
+ .cache()
+ ) # cache to avoid Constant Folding Optimization
+ df = df.select(ExpandAddress("address").alias("normalized"))
+ # Actually running downloads the model and is very expensive, so we just check the plan
+ # Checking the plan should allow us to verify that the function is correctly registered
+ df.explain()
Review Comment:
let's try
--
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-2074] Add libpostal integration [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2201911645
##########
python/tests/sql/test_dataframe_api.py:
##########
@@ -1783,3 +1783,29 @@ def test_lof(self):
df.withColumn(
"localOutlierFactor", ST_LocalOutlierFactor("geometry", 2, False)
).collect()
+
+ def test_expand_address_df_api(self):
+ df = (
+ self.spark.range(1)
+ .selectExpr(
+ "'781 Franklin Ave Crown Heights Brooklyn NY 11216 USA' as address"
+ )
+ .cache()
+ ) # cache to avoid Constant Folding Optimization
+ df = df.select(ExpandAddress("address").alias("normalized"))
+ # Actually running downloads the model and is very expensive, so we just check the plan
+ # Checking the plan should allow us to verify that the function is correctly registered
+ df.explain()
+
+ def test_parse_address_df_api(self):
+ df = (
+ self.spark.range(1)
+ .selectExpr(
+ "'781 Franklin Ave Crown Heights Brooklyn NY 11216 USA' as address"
+ )
+ .cache()
Review Comment:
Why is the cache needed?
--
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-2074] Add libpostal integration [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on PR #2077:
URL: https://github.com/apache/sedona/pull/2077#issuecomment-3059824827
Jpostal requires Java 17? Sedona is compiled against Java 11. Will this even pass our Java tests in CI?
--
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-2074] Add libpostal integration [sedona]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on PR #2077:
URL: https://github.com/apache/sedona/pull/2077#issuecomment-3055406915
@Imbruced Any idea what might be causing the test failure in this new version I added to the test matrix?
spark 3.5.4, java 17, scala 2.12.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
Re: [PR] [GH-2074] Add libpostal integration [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on PR #2077:
URL: https://github.com/apache/sedona/pull/2077#issuecomment-3063913481
Please also remove all `show` and `print` command
--
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-2074] Add libpostal integration [sedona]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2201911070
##########
pom.xml:
##########
@@ -422,6 +422,18 @@
<id>Spark Packages</id>
<url>https://repos.spark-packages.org/</url>
</repository>
+<!-- TODO remove me and use jpostal actual release-->
+ <repository>
Review Comment:
yes only just now did I finish publishing the release version
--
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-2074] Add libpostal integration [sedona]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2205614398
##########
python/tests/sql/test_dataframe_api.py:
##########
@@ -1783,3 +1783,29 @@ def test_lof(self):
df.withColumn(
"localOutlierFactor", ST_LocalOutlierFactor("geometry", 2, False)
).collect()
+
+ def test_expand_address_df_api(self):
+ df = (
+ self.spark.range(1)
+ .selectExpr(
+ "'781 Franklin Ave Crown Heights Brooklyn NY 11216 USA' as address"
+ )
+ .cache()
+ ) # cache to avoid Constant Folding Optimization
+ df = df.select(ExpandAddress("address").alias("normalized"))
+ # Actually running downloads the model and is very expensive, so we just check the plan
+ # Checking the plan should allow us to verify that the function is correctly registered
+ df.explain()
Review Comment:
This test is to make sure that the function can be resolved when using the dataframe API in python so rewriting it into a scala test is not helpful.
Maybe I can use [sameSemantics](https://spark.apache.org/docs/latest/api/python/reference/pyspark.sql/api/pyspark.sql.DataFrame.sameSemantics.html#pyspark.sql.DataFrame.sameSemantics) to accomplish this:
```python
df.select(ExpandAddress("address").alias("normalized")).sameSemantics(
df.select(f.expr("ExpandAddress(address)").alias("normalized"))
)
```
--
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-2074] Add libpostal integration [sedona]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2193967099
##########
python/sedona/spark/sql/st_functions.py:
##########
@@ -20,7 +20,7 @@
from typing import Optional, Union
from pyspark.sql import Column
-from pyspark.sql.functions import lit
+from pyspark.sql.functions import lit, col
Review Comment:
Wrong
--
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-2074] Add libpostal integration [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2201911351
##########
python/tests/sql/test_dataframe_api.py:
##########
@@ -1783,3 +1783,29 @@ def test_lof(self):
df.withColumn(
"localOutlierFactor", ST_LocalOutlierFactor("geometry", 2, False)
).collect()
+
+ def test_expand_address_df_api(self):
+ df = (
+ self.spark.range(1)
+ .selectExpr(
+ "'781 Franklin Ave Crown Heights Brooklyn NY 11216 USA' as address"
+ )
+ .cache()
+ ) # cache to avoid Constant Folding Optimization
+ df = df.select(ExpandAddress("address").alias("normalized"))
+ # Actually running downloads the model and is very expensive, so we just check the plan
+ # Checking the plan should allow us to verify that the function is correctly registered
+ df.explain()
Review Comment:
Add a test case to actually check the plan?
--
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-2074] Add libpostal integration [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2201910327
##########
pom.xml:
##########
@@ -422,6 +422,18 @@
<id>Spark Packages</id>
<url>https://repos.spark-packages.org/</url>
</repository>
+<!-- TODO remove me and use jpostal actual release-->
+ <repository>
Review Comment:
Remove this?
--
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-2074] Add libpostal integration [sedona]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2201938169
##########
python/tests/sql/test_dataframe_api.py:
##########
@@ -1783,3 +1783,29 @@ def test_lof(self):
df.withColumn(
"localOutlierFactor", ST_LocalOutlierFactor("geometry", 2, False)
).collect()
+
+ def test_expand_address_df_api(self):
+ df = (
+ self.spark.range(1)
+ .selectExpr(
+ "'781 Franklin Ave Crown Heights Brooklyn NY 11216 USA' as address"
+ )
+ .cache()
+ ) # cache to avoid Constant Folding Optimization
+ df = df.select(ExpandAddress("address").alias("normalized"))
+ # Actually running downloads the model and is very expensive, so we just check the plan
+ # Checking the plan should allow us to verify that the function is correctly registered
+ df.explain()
+
+ def test_parse_address_df_api(self):
+ df = (
+ self.spark.range(1)
+ .selectExpr(
+ "'781 Franklin Ave Crown Heights Brooklyn NY 11216 USA' as address"
+ )
+ .cache()
Review Comment:
to avoid constant folding causing the evaluation of the statement in the logical optimizer. without the cache the function call gets evaluated which we are trying to avoid.
--
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-2074] Add libpostal integration [sedona]
Posted by "james-willis (via GitHub)" <gi...@apache.org>.
james-willis commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2201940089
##########
pom.xml:
##########
@@ -422,6 +422,18 @@
<id>Spark Packages</id>
<url>https://repos.spark-packages.org/</url>
</repository>
+<!-- TODO remove me and use jpostal actual release-->
+ <repository>
Review Comment:
This is fixed. I only had this here to test that Java 11 version of jpostal before I made a non-snapshot release of it.
--
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-2074] Add libpostal integration [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2205487634
##########
python/tests/sql/test_dataframe_api.py:
##########
@@ -1783,3 +1783,29 @@ def test_lof(self):
df.withColumn(
"localOutlierFactor", ST_LocalOutlierFactor("geometry", 2, False)
).collect()
+
+ def test_expand_address_df_api(self):
+ df = (
+ self.spark.range(1)
+ .selectExpr(
+ "'781 Franklin Ave Crown Heights Brooklyn NY 11216 USA' as address"
+ )
+ .cache()
+ ) # cache to avoid Constant Folding Optimization
+ df = df.select(ExpandAddress("address").alias("normalized"))
+ # Actually running downloads the model and is very expensive, so we just check the plan
+ # Checking the plan should allow us to verify that the function is correctly registered
+ df.explain()
Review Comment:
If not, then check it in the Scala side? See this approach: https://github.com/apache/sedona/blob/77685486a13ce446bbe9887089f7d16b912cc94a/spark/common/src/test/scala/org/apache/sedona/sql/predicateJoinTestScala.scala#L56
I want to avoid spitting content to stdout as much as possible. I made lots of these mistakes in Sedona source code but want to clean it up at some point.
--
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-2074] Add libpostal integration [sedona]
Posted by "jiayuasu (via GitHub)" <gi...@apache.org>.
jiayuasu commented on code in PR #2077:
URL: https://github.com/apache/sedona/pull/2077#discussion_r2205478555
##########
python/tests/sql/test_dataframe_api.py:
##########
@@ -1783,3 +1783,29 @@ def test_lof(self):
df.withColumn(
"localOutlierFactor", ST_LocalOutlierFactor("geometry", 2, False)
).collect()
+
+ def test_expand_address_df_api(self):
+ df = (
+ self.spark.range(1)
+ .selectExpr(
+ "'781 Franklin Ave Crown Heights Brooklyn NY 11216 USA' as address"
+ )
+ .cache()
+ ) # cache to avoid Constant Folding Optimization
+ df = df.select(ExpandAddress("address").alias("normalized"))
+ # Actually running downloads the model and is very expensive, so we just check the plan
+ # Checking the plan should allow us to verify that the function is correctly registered
+ df.explain()
+
+ def test_parse_address_df_api(self):
+ df = (
+ self.spark.range(1)
+ .selectExpr(
+ "'781 Franklin Ave Crown Heights Brooklyn NY 11216 USA' as address"
+ )
+ .cache()
Review Comment:
Please uncache it once it is done
--
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