[SEDONA-625] Add ST_GeneratePoints #1520

furqaankhan · 2024-07-10T22:21:03Z

Did you read the Contributor Guide?

Yes, I have read the Contributor Rules and Contributor Development Guide

Is this PR related to a JIRA ticket?

Yes, the URL of the associated JIRA ticket is https://issues.apache.org/jira/browse/SEDONA-XXX. The PR name follows the format [SEDONA-XXX] my subject.

What changes were proposed in this PR?

Add ST_GeneratePoints

How was this patch tested?

Add new unit tests

Did this PR include necessary documentation updates?

Yes, I have updated the documentation.

jiayuasu · 2024-07-11T07:18:23Z

@Kontinuation The result of this function is non-deterministic. Will this create a problem to the catalyst optimizer?

Kontinuation · 2024-07-11T10:50:07Z

@Kontinuation The result of this function is non-deterministic. Will this create a problem to the catalyst optimizer?

The correct way to implement non-deterministic functions is to extend the Nondeterministic trait and implement the initializeInternal method to calculate the seed for each partition. The function evaluates using the initialized seed, so the results are identical when partitions are recomputed. Please refer to randomExpressions.scala#L93 to understand how this works.

Unfortunately, RandomPointsBuilder does not allow constructing the builder with a given seed. We have to inherit this class and override the createRandomCoord function to generate coordinates using the specified seed.

jiayuasu · 2024-07-11T17:34:51Z

@furqaankhan can you solve the issue using Kristin's method?

furqaankhan · 2024-07-11T17:36:05Z

Yes, I am working on it.

jiayuasu · 2024-07-14T02:00:37Z

This PR should also add SRID to the generated points following the style of this PR: #1521

common/src/main/java/org/apache/sedona/common/utils/RandomPointsBuilderSeed.java

Kontinuation · 2024-07-15T13:00:41Z

spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Functions.scala

+ override protected def initializeInternal(partitionIndex: Int): Unit = random = new Random(
+ randomSeed.getOrElse(0L) + partitionIndex)
+
+ override protected def evalInternal(input: InternalRow): Any = {
+ val geom = children.head.toGeometry(input)
+ val numPoints = children(1).eval(input).asInstanceOf[Int]
+ if (nArgs == 3) {
+ val seed = children(2).eval(input).asInstanceOf[Int]
+ return GeometrySerializer.serialize(Functions.generatePoints(geom, numPoints, seed))
+ }
+ GeometrySerializer.serialize(Functions.generatePoints(geom, numPoints))
+ }


We should construct an instance of RandomPointsBuilderSeed in the initializeInternal function, and call setExtent, setNumPoints, getGeometry methods using that instance in evalInternal.

Kontinuation · 2024-07-15T13:21:22Z

spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Functions.scala

+case class ST_GeneratePoints(inputExpressions: Seq[Expression], randomSeed: Option[Long] = None)
+ extends Expression
+ with Nondeterministic
+ with ExpectsInputTypes
+ with CodegenFallback
+ with ExpressionWithRandomSeed {
+
+ def this(inputExpressions: Seq[Expression]) = this(inputExpressions, Some(0L))


ExpressionWithRandomSeed has binary incompatible changes since Spark 3.0, so we cannot use it to write binary compatible code for Spark 3.0 ~ 3.5. Maintaining compatibility with old Spark versions (<= 3.2) is really a burden for us.

We can initialize ST_GeneratePoints in the old Spark 2.3 way:

case class ST_GeneratePoints(inputExpressions: Seq[Expression], randomSeed: Long) extends RDG { def this(inputExpressions: Seq[Expression]) = this(inputExpressions, Utils.random.nextLong())

The consequence is that the random seed will be fixed for multiple minibatches when running streaming jobs. However, that's the best we can do to maintain compatibility with various Spark versions.

Using RDG doesn't work, as it extends UnaryExpression. Hence, I used all the traits that RDG was using but replaced the UnaryExpression with Expression. Please let me know if you think otherwise

Ah, I made a mistake when writing the example code. It should be

case class ST_GeneratePoints(inputExpressions: Seq[Expression], randomSeed: Long) extends Expression with Nondeterministic with ExpectsInputTypes with CodegenFallback { def this(inputExpressions: Seq[Expression]) = this(inputExpressions, Utils.random.nextLong()) }

I was going to push this changes by removing the ExpressionWithRandomSeed trait and it's overridden members, to test it.

furqaankhan · 2024-07-15T16:39:48Z

This PR should also add SRID to the generated points following the style of this PR: #1521

I am using the factory of the input geometry. that will preserve the SRID. Please let me know if you think otherwise.

https://github.com/furqaankhan/sedona/blob/919136e2d67e730bd864f87c466a7f8be3b84407/common/src/main/java/org/apache/sedona/common/Functions.java#L1808

jiayuasu · 2024-07-16T04:36:46Z

@furqaankhan Can you create a PR on snowflake-tester?

furqaankhan · 2024-07-16T14:44:29Z

I did create one, I closed it as it had passed. I reopened it as I added a new function signature. The PR.

jiayuasu · 2024-07-17T20:55:31Z

common/src/main/java/org/apache/sedona/common/Functions.java

+ return generatePoints(geom, numPoints, 0);
+ }
+
+ public static Geometry generatePoints(Geometry geom, int numPoints, Random random) {


Why is there another overloaded function that takes Random as input?

This is for using the random member of ST_GeneratePoints to generate consistent random streams

jiayuasu · 2024-07-17T20:56:45Z

spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Functions.scala

+ if (seed > 0) {
+ Functions.generatePoints(geom, numPoints, seed)
+ } else {
+ Functions.generatePoints(geom, numPoints, random)


Why is this needed? This if condition seems to be repeating the same condition in Functions

This is for generating consistent random streams even if seed is 0.

furqaankhan requested a review from jiayuasu as a code owner July 10, 2024 22:21

github-actions bot added sedona-python docs sedona-flink sedona-common sedona-spark sedona-snowflake labels Jul 10, 2024

jiayuasu approved these changes Jul 11, 2024

View reviewed changes

jiayuasu added this to the sedona-1.6.1 milestone Jul 11, 2024

jiayuasu added the improvement label Jul 11, 2024

furqaankhan requested review from jiayuasu and Kontinuation July 12, 2024 18:00

Kontinuation reviewed Jul 15, 2024

View reviewed changes

jiayuasu requested a review from Kontinuation July 16, 2024 22:02

furqaankhan added 7 commits July 18, 2024 03:19

feat: add ST_GeneratePoints

edab5c8

fix: lint errors

ac3bffb

fix: snowflake v2 registeration

c6de12e

feat: add Nondeterministic trait to spark

ba33b3c

fix: optimize imports

c996fe8

fix: optimize imports and spark registration

83c98d7

fix: spark registration 1

23b1843

furqaankhan and others added 7 commits July 18, 2024 03:19

fix: spark registration 2 and remove unnecessary code

1e578f8

fix: spark registration 3

9e22c5b

fix: remove ExpressionWithRandomSeed

39087df

docs: add new seed parameter to docs

0925e74

chore: add the new parameter to all engines and add tests

7f63a99

fix: snowflake test

0b40b5c

Use per-partition random number generator when seed is not specified.

9d47bad

Kontinuation force-pushed the st-generate-points branch from 3c9d48b to 9d47bad Compare July 17, 2024 19:35

jiayuasu requested changes Jul 17, 2024

View reviewed changes

jiayuasu approved these changes Jul 17, 2024

View reviewed changes

jiayuasu merged commit 1f24ca6 into apache:master Jul 17, 2024
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SEDONA-625] Add ST_GeneratePoints #1520

[SEDONA-625] Add ST_GeneratePoints #1520

furqaankhan commented Jul 10, 2024

jiayuasu commented Jul 11, 2024

Kontinuation commented Jul 11, 2024 •

edited

Loading

jiayuasu commented Jul 11, 2024

furqaankhan commented Jul 11, 2024

jiayuasu commented Jul 14, 2024

Kontinuation Jul 15, 2024

Kontinuation Jul 15, 2024

furqaankhan Jul 15, 2024 •

edited

Loading

Kontinuation Jul 16, 2024

furqaankhan Jul 16, 2024

furqaankhan commented Jul 15, 2024

jiayuasu commented Jul 16, 2024

furqaankhan commented Jul 16, 2024

jiayuasu Jul 17, 2024

Kontinuation Jul 17, 2024

jiayuasu Jul 17, 2024

Kontinuation Jul 17, 2024

[SEDONA-625] Add ST_GeneratePoints #1520

[SEDONA-625] Add ST_GeneratePoints #1520

Conversation

furqaankhan commented Jul 10, 2024

Did you read the Contributor Guide?

Is this PR related to a JIRA ticket?

What changes were proposed in this PR?

How was this patch tested?

Did this PR include necessary documentation updates?

jiayuasu commented Jul 11, 2024

Kontinuation commented Jul 11, 2024 • edited Loading

jiayuasu commented Jul 11, 2024

furqaankhan commented Jul 11, 2024

jiayuasu commented Jul 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furqaankhan Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furqaankhan commented Jul 15, 2024

jiayuasu commented Jul 16, 2024

furqaankhan commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kontinuation commented Jul 11, 2024 •

edited

Loading

furqaankhan Jul 15, 2024 •

edited

Loading