[GitHub] spark pull request #20861: [SPARK-23599][SQL] Use RandomUUIDGenerator in Uui...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20861#discussion_r205540393
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
---
@@ -1994,6 +1996,20 @@ class Analyzer(
}
}
+ /**
+ * Set the seed for random number generation in Uuid expressions.
+ */
+ object ResolvedUuidExpressions extends Rule[LogicalPlan] {
+private lazy val random = new Random()
+
+override def apply(plan: LogicalPlan): LogicalPlan = plan.transformUp {
+ case p if p.resolved => p
+ case p => p transformExpressionsUp {
+case Uuid(None) => Uuid(Some(random.nextLong()))
--- End diff --
I created a PR #21854 which shows behavior of `Uuid` in streaming. I think
rand should be the same.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20861: [SPARK-23599][SQL] Use RandomUUIDGenerator in Uui...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20861#discussion_r205337069
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
---
@@ -1994,6 +1996,20 @@ class Analyzer(
}
}
+ /**
+ * Set the seed for random number generation in Uuid expressions.
+ */
+ object ResolvedUuidExpressions extends Rule[LogicalPlan] {
+private lazy val random = new Random()
+
+override def apply(plan: LogicalPlan): LogicalPlan = plan.transformUp {
+ case p if p.resolved => p
+ case p => p transformExpressionsUp {
+case Uuid(None) => Uuid(Some(random.nextLong()))
--- End diff --
what's the current behavior for rand in streaming?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20861: [SPARK-23599][SQL] Use RandomUUIDGenerator in Uui...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20861#discussion_r204272567
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
---
@@ -1994,6 +1996,20 @@ class Analyzer(
}
}
+ /**
+ * Set the seed for random number generation in Uuid expressions.
+ */
+ object ResolvedUuidExpressions extends Rule[LogicalPlan] {
+private lazy val random = new Random()
+
+override def apply(plan: LogicalPlan): LogicalPlan = plan.transformUp {
+ case p if p.resolved => p
+ case p => p transformExpressionsUp {
+case Uuid(None) => Uuid(Some(random.nextLong()))
--- End diff --
One thing I think is, is any use case that we need to re-initialize random
seed for `Rand`? Maybe streaming? For streaming query, I think `Rand` should
use different random seed in each execution. For now, the random seed is
initialized when constructing, even we re-analyze the query, it still uses same
seed.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20861: [SPARK-23599][SQL] Use RandomUUIDGenerator in Uui...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20861#discussion_r204266884
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
---
@@ -1994,6 +1996,20 @@ class Analyzer(
}
}
+ /**
+ * Set the seed for random number generation in Uuid expressions.
+ */
+ object ResolvedUuidExpressions extends Rule[LogicalPlan] {
+private lazy val random = new Random()
+
+override def apply(plan: LogicalPlan): LogicalPlan = plan.transformUp {
+ case p if p.resolved => p
+ case p => p transformExpressionsUp {
+case Uuid(None) => Uuid(Some(random.nextLong()))
--- End diff --
I have checked, and actually `Rand` and `Randn` already have this behavior
that they are deterministic between re-tries, though their random seeds are not
determined at analysis but at constructing.
So I'm thinking should we do the same thing (random seed initialized at
analysis) to `Rand` and `Randn`? Besides just to be consistent with `Uuid`, is
any good reason to do this?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20861: [SPARK-23599][SQL] Use RandomUUIDGenerator in Uui...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20861#discussion_r204242507
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
---
@@ -1994,6 +1996,20 @@ class Analyzer(
}
}
+ /**
+ * Set the seed for random number generation in Uuid expressions.
+ */
+ object ResolvedUuidExpressions extends Rule[LogicalPlan] {
+private lazy val random = new Random()
+
+override def apply(plan: LogicalPlan): LogicalPlan = plan.transformUp {
+ case p if p.resolved => p
+ case p => p transformExpressionsUp {
+case Uuid(None) => Uuid(Some(random.nextLong()))
--- End diff --
SGTM. We can even create a base trait for these random functions.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20861: [SPARK-23599][SQL] Use RandomUUIDGenerator in Uui...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20861#discussion_r204201930
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
---
@@ -1994,6 +1996,20 @@ class Analyzer(
}
}
+ /**
+ * Set the seed for random number generation in Uuid expressions.
+ */
+ object ResolvedUuidExpressions extends Rule[LogicalPlan] {
+private lazy val random = new Random()
+
+override def apply(plan: LogicalPlan): LogicalPlan = plan.transformUp {
+ case p if p.resolved => p
+ case p => p transformExpressionsUp {
+case Uuid(None) => Uuid(Some(random.nextLong()))
--- End diff --
hmm, if we want to make it deterministic between re-tries of same query. I
think we should do it. I can make a PR for it, WDYT?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20861: [SPARK-23599][SQL] Use RandomUUIDGenerator in Uui...
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/20861#discussion_r203968743
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
---
@@ -1994,6 +1996,20 @@ class Analyzer(
}
}
+ /**
+ * Set the seed for random number generation in Uuid expressions.
+ */
+ object ResolvedUuidExpressions extends Rule[LogicalPlan] {
+private lazy val random = new Random()
+
+override def apply(plan: LogicalPlan): LogicalPlan = plan.transformUp {
+ case p if p.resolved => p
+ case p => p transformExpressionsUp {
+case Uuid(None) => Uuid(Some(random.nextLong()))
--- End diff --
shall we do the same thing for `Rand`?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20861: [SPARK-23599][SQL] Use RandomUUIDGenerator in Uui...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/20861 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20861: [SPARK-23599][SQL] Use RandomUUIDGenerator in Uui...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20861#discussion_r175727781
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolvedUuidExpressionsSuite.scala
---
@@ -0,0 +1,79 @@
+/*
+ * 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.spark.sql.catalyst.analysis
+
+import scala.collection.mutable.ArrayBuffer
+
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.dsl.plans._
+import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{LocalRelation,
LogicalPlan}
+
+/**
+ * Test suite for resolving Uuid expressions.
+ */
+class ResolvedUuidExpressionsSuite extends AnalysisTest {
+
+ private lazy val a = 'a.int
+ private lazy val r = LocalRelation(a)
+ private lazy val uuid1 = Uuid().as('_uuid1)
+ private lazy val uuid2 = Uuid().as('_uuid2)
+ private lazy val uuid3 = Uuid().as('_uuid3)
+ private lazy val uuid1Ref = uuid1.toAttribute
+
+ private val analyzer = getAnalyzer(caseSensitive = true)
+
+ private def getUuidExpressions(plan: LogicalPlan): Seq[Uuid] = {
+val uuids = new ArrayBuffer[Uuid]()
+plan.transformUp {
--- End diff --
Nit use `flatMap`?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20861: [SPARK-23599][SQL] Use RandomUUIDGenerator in Uui...
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/20861#discussion_r175727739
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolvedUuidExpressionsSuite.scala
---
@@ -0,0 +1,79 @@
+/*
+ * 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.spark.sql.catalyst.analysis
+
+import scala.collection.mutable.ArrayBuffer
+
+import org.apache.spark.sql.catalyst.dsl.expressions._
+import org.apache.spark.sql.catalyst.dsl.plans._
+import org.apache.spark.sql.catalyst.expressions._
+import org.apache.spark.sql.catalyst.plans.logical.{LocalRelation,
LogicalPlan}
+
+/**
+ * Test suite for resolving Uuid expressions.
+ */
+class ResolvedUuidExpressionsSuite extends AnalysisTest {
+
+ private lazy val a = 'a.int
+ private lazy val r = LocalRelation(a)
+ private lazy val uuid1 = Uuid().as('_uuid1)
+ private lazy val uuid2 = Uuid().as('_uuid2)
+ private lazy val uuid3 = Uuid().as('_uuid3)
+ private lazy val uuid1Ref = uuid1.toAttribute
+
+ private val analyzer = getAnalyzer(caseSensitive = true)
+
+ private def getUuidExpressions(plan: LogicalPlan): Seq[Uuid] = {
+val uuids = new ArrayBuffer[Uuid]()
+plan.transformUp {
+ case p =>
+p.transformExpressionsUp {
--- End diff --
NIT: use `collect`?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20861: [SPARK-23599][SQL] Use RandomUUIDGenerator in Uui...
Github user hvanhovell commented on a diff in the pull request: https://github.com/apache/spark/pull/20861#discussion_r175725276 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala --- @@ -98,6 +99,8 @@ class Analyzer( this(catalog, conf, conf.optimizerMaxIterations) } + private lazy val random = new Random() --- End diff -- Shall we put `random` in the `ResolvedUuidExpressions`? That makes it a little bit easier to follow. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20861: [SPARK-23599][SQL] Use RandomUUIDGenerator in Uui...
GitHub user viirya opened a pull request: https://github.com/apache/spark/pull/20861 [SPARK-23599][SQL] Use RandomUUIDGenerator in Uuid expression ## What changes were proposed in this pull request? As stated in Jira, there are problems with current `Uuid` expression which uses `java.util.UUID.randomUUID` for UUID generation. This patch uses the newly added `RandomUUIDGenerator` for UUID generation. So we can make `Uuid` deterministic between retries. ## How was this patch tested? Added unit tests. You can merge this pull request into a Git repository by running: $ git pull https://github.com/viirya/spark-1 SPARK-23599-2 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/20861.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #20861 commit 306dbe8e26f2045b0d133e07455dedae058c0311 Author: Liang-Chi Hsieh Date: 2018-03-20T03:11:33Z Use new UUID generator in Uuid expression. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
