[SPARK-33857][SQL] Unify the default seed of random functions #30864

ulysses-you · 2020-12-20T12:40:37Z

What changes were proposed in this pull request?

Unify the seed of random functions

Add a hold place expression UnresolvedSeed as the defualt seed.
Change Rand,Randn,Uuid,Shuffle default seed to UnresolvedSeed .
Replace UnresolvedSeed to real seed at ResolveRandomSeed rule.

Why are the changes needed?

Uuid and Shuffle use the ResolveRandomSeed rule to set the seed if user doesn't give a seed value. Rand and Randn do this at constructing.

It's better to unify the default seed at Analyzer side since we have used ExpressionWithRandomSeed at streaming query.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass exists test and add test.

SparkQA · 2020-12-20T13:50:11Z

Test build #133103 has finished for PR 30864 at commit 29b5d1e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Shuffle(child: Expression, randomSeed: Expression = DefaultSeed())
case class Uuid(randomSeed: Expression = DefaultSeed()) extends LeafExpression with Stateful
trait ExpressionWithRandomSeed extends Expression
case class DefaultSeed() extends Expression with Unevaluable
case class Rand(child: Expression = DefaultSeed(), hideSeed: Boolean = false)
case class Randn(child: Expression = DefaultSeed(), hideSeed: Boolean = false)

SparkQA · 2020-12-21T02:36:19Z

Test build #133119 has finished for PR 30864 at commit 599d6d9.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-21T04:09:16Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37725/

SparkQA · 2020-12-21T04:38:01Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37725/

SparkQA · 2020-12-21T06:12:01Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37729/

SparkQA · 2020-12-21T06:40:25Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37729/

SparkQA · 2020-12-21T07:15:27Z

Test build #133126 has finished for PR 30864 at commit 620692c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-21T09:17:33Z

Test build #133131 has finished for PR 30864 at commit 95276c7.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-21T09:29:10Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37736/

SparkQA · 2020-12-21T09:57:16Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37736/

HyukjinKwon · 2020-12-21T10:53:54Z

retest this please

SparkQA · 2020-12-21T12:18:57Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37746/

SparkQA · 2020-12-21T12:56:19Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37746/

SparkQA · 2020-12-21T13:34:49Z

Test build #133137 has finished for PR 30864 at commit 0ed9ca3.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Rand(child: Expression, hideSeed: Boolean = false)
case class Randn(child: Expression, hideSeed: Boolean = false)

SparkQA · 2020-12-21T16:03:18Z

Test build #133147 has finished for PR 30864 at commit 0ed9ca3.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Rand(child: Expression, hideSeed: Boolean = false)
case class Randn(child: Expression, hideSeed: Boolean = false)

maropu · 2020-12-22T00:13:26Z

Just a question; any system having uuid with a seed param? I couldn't find it though.

ulysses-you · 2020-12-22T01:34:00Z

@maropu thanks for the thought.

any system having uuid with a seed param?

I haven't seen it too, but before this PR Spark has already support the seed param.

I'd say the main change of this PR is to keep the seed initialize at Analyzer side. For this idea, we make all the rand functions (include uuid) have the same seed param.

maropu · 2020-12-22T02:13:55Z

Ah, I see. If so, I think its better to focus on the refactoring for the seed initialization and not to support the new feature: select uuid(1) in this PR.

dongjoon-hyun

This seems too intrusive for the goal; select uuid(1). Could you simplify this PR?

dongjoon-hyun · 2020-12-22T04:30:01Z

Oh, I commented before reading @maropu 's last comment. Ya. This looks like two orthogonal purposes.

ulysses-you · 2020-12-22T06:17:48Z

thanks @maropu @dongjoon-hyun , will to narrow the goal and make this PR focus on one thing.

SparkQA · 2020-12-22T11:24:31Z

Test build #133203 has finished for PR 30864 at commit 786b9d7.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class DefaultSeed() extends Expression with Unevaluable
case class Rand(child: Expression, hideSeed: Boolean = false) extends RDG
case class Randn(child: Expression, hideSeed: Boolean = false) extends RDG

SparkQA · 2020-12-22T11:33:24Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37807/

maropu · 2020-12-22T12:02:09Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/randomExpressions.scala

+/**
+ * A place holder expression used in random functions, will be replaced after analyze.
+ */
+case class DefaultSeed() extends Expression with Unevaluable {


case object UnresolvedSeed extends Expression with Unevaluable?

yes, it's better.

SparkQA · 2020-12-22T12:05:52Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37807/

SparkQA · 2020-12-22T12:25:22Z

Test build #133209 has finished for PR 30864 at commit d66af0d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-22T15:18:34Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37822/

SparkQA · 2020-12-22T15:57:43Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37822/

SparkQA · 2020-12-22T19:30:57Z

Test build #133225 has finished for PR 30864 at commit d8237c3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-23T06:49:18Z

Test build #133266 has finished for PR 30864 at commit 8d39f5a.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-23T07:29:35Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37865/

SparkQA · 2020-12-23T07:58:56Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37865/

SparkQA · 2020-12-23T10:47:26Z

Test build #133271 has finished for PR 30864 at commit edf0b03.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

ulysses-you · 2020-12-23T11:35:12Z

I think it's ok to review this. cc @maropu @dongjoon-hyun @cloud-fan

dongjoon-hyun · 2020-12-23T23:58:56Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/randomExpressions.scala


  def this(child: Expression) = this(child, false)

-  override def withNewSeed(seed: Long): Rand = Rand(Literal(seed, LongType))
+  override def withNewSeed(seed: Long): Rand = Rand(Literal(seed, LongType), hideSeed)


Ur, is this related to this PR, @ulysses-you ? This looks like an orthogonal bug fix a little to me. I'm wondering if we we have a test coverage for this.

@dongjoon-hyun thanks for the carefully check. Yes, it could be thought as bug fix. See the jenkins test result before this commit a3c9f0b.

And I think for early branch, we just need to backport this place and leave this PR for master. But also, I'm OK to send a PR to fix this first, then go back to this PR.

This is an orthogonal bug fix, but it only affects the sql method. I don't have a strong opinion about backporting the fix or not.

Got it. Thanks, @cloud-fan .

dongjoon-hyun · 2020-12-23T23:59:05Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/randomExpressions.scala


  def this(child: Expression) = this(child, false)

-  override def withNewSeed(seed: Long): Randn = Randn(Literal(seed, LongType))
+  override def withNewSeed(seed: Long): Randn = Randn(Literal(seed, LongType), hideSeed)


dongjoon-hyun

In general, this PR looks good, but I have a question on the change because SPARK-33857 is an improvement PR which lands only at master branch.

If we are correcting some fix in this PR, we should split this PR by filing a new JIRA for that and backport to the release branches.

dongjoon-hyun

+1, LGTM. Merged to master for Apache Spark 3.2.0.
Thank you, @ulysses-you and all.

Merry Christmas and Happy New Year!

maropu · 2020-12-25T00:10:21Z

late lgtm.

ulysses-you · 2020-12-25T01:05:56Z

thank you all ! Merry Christmas !

### What changes were proposed in this pull request? Unify the seed of random functions 1. Add a hold place expression `UnresolvedSeed ` as the defualt seed. 2. Change `Rand`,`Randn`,`Uuid`,`Shuffle` default seed to `UnresolvedSeed `. 3. Replace `UnresolvedSeed ` to real seed at `ResolveRandomSeed` rule. ### Why are the changes needed? `Uuid` and `Shuffle` use the `ResolveRandomSeed` rule to set the seed if user doesn't give a seed value. `Rand` and `Randn` do this at constructing. It's better to unify the default seed at Analyzer side since we have used `ExpressionWithRandomSeed` at streaming query. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass exists test and add test. Closes apache#30864 from ulysses-you/SPARK-33857. Authored-by: ulysses-you <ulyssesyou18@gmail.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

github-actions bot added the SQL label Dec 20, 2020

dongjoon-hyun reviewed Dec 22, 2020

View reviewed changes

ulysses-you force-pushed the SPARK-33857 branch from 665cb53 to 0ed9ca3 Compare December 22, 2020 08:58

refactor

786b9d7

ulysses-you force-pushed the SPARK-33857 branch from 0ed9ca3 to 786b9d7 Compare December 22, 2020 09:06

ulysses-you changed the title ~~[SPARK-33857][SQL] Unify random functions and make Uuid Shuffle support seed in SQL~~ [SPARK-33857][SQL] Unify the default seed of random functions Dec 22, 2020

fix

d66af0d

maropu reviewed Dec 22, 2020

View reviewed changes

ulysses-you added 3 commits December 22, 2020 22:25

object

5573721

hideSeed

a3c9f0b

UnresolvedSeed

d8237c3

ulysses-you added 2 commits December 23, 2020 14:02

move UnresolvedSeed

8d39f5a

cleanup

edf0b03

cloud-fan approved these changes Dec 23, 2020

View reviewed changes

dongjoon-hyun reviewed Dec 23, 2020

View reviewed changes

dongjoon-hyun reviewed Dec 24, 2020

View reviewed changes

dongjoon-hyun approved these changes Dec 24, 2020

View reviewed changes

dongjoon-hyun closed this in 9c30116 Dec 24, 2020

[SPARK-33857][SQL] Unify the default seed of random functions #30864

[SPARK-33857][SQL] Unify the default seed of random functions #30864

Uh oh!

Conversation

ulysses-you commented Dec 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Dec 20, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

HyukjinKwon commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

SparkQA commented Dec 21, 2020

Uh oh!

maropu commented Dec 22, 2020

Uh oh!

ulysses-you commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maropu commented Dec 22, 2020

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Dec 22, 2020

Uh oh!

ulysses-you commented Dec 22, 2020

Uh oh!

SparkQA commented Dec 22, 2020

Uh oh!

SparkQA commented Dec 22, 2020

Uh oh!

maropu Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

ulysses-you Dec 22, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 22, 2020

Uh oh!

SparkQA commented Dec 22, 2020

Uh oh!

SparkQA commented Dec 22, 2020

Uh oh!

SparkQA commented Dec 22, 2020

Uh oh!

SparkQA commented Dec 22, 2020

Uh oh!

SparkQA commented Dec 23, 2020

Uh oh!

SparkQA commented Dec 23, 2020

Uh oh!

SparkQA commented Dec 23, 2020

ulysses-you commented Dec 20, 2020 •

edited

Loading

ulysses-you commented Dec 22, 2020 •

edited

Loading

ulysses-you Dec 24, 2020 •

edited

Loading