[SPARK-32793][SQL] Add raise_error function, adds error message parameter to assert_true #29947

karenfeng · 2020-10-05T21:34:26Z

What changes were proposed in this pull request?

Adds a SQL function raise_error which underlies the refactored assert_true function. assert_true now also (optionally) accepts a custom error message field.
raise_error is exposed in SQL, Python, Scala, and R.
assert_true was previously only exposed in SQL; it is now also exposed in Python, Scala, and R.

Why are the changes needed?

Improves usability of assert_true by clarifying error messaging, and adds the useful helper function raise_error.

Does this PR introduce any user-facing change?

Yes:

Adds raise_error function to the SQL, Python, Scala, and R APIs.
Adds assert_true function to the SQL, Python and R APIs.

How was this patch tested?

Adds unit tests in SQL, Python, Scala, and R for assert_true and raise_error.

Signed-off-by: Karen Feng <karen.feng@databricks.com>

…2793 Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2020-10-05T21:40:17Z

Test build #129422 has finished for PR 29947 at commit df1fc36.

This patch fails Python style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class DisableUnnecessaryBucketedScan(conf: SQLConf) extends Rule[SparkPlan]
abstract class JdbcConnectionProvider

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2020-10-05T22:01:10Z

Test build #129423 has finished for PR 29947 at commit 0ff60f4.

This patch fails R style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-10-05T22:19:09Z

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

SparkQA · 2020-10-05T22:30:31Z

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

SparkQA · 2020-10-05T22:35:39Z

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

SparkQA · 2020-10-05T22:49:34Z

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

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2020-10-05T23:47:16Z

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

SparkQA · 2020-10-06T00:04:51Z

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

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionSuite.scala

SparkQA · 2020-10-06T00:50:42Z

Test build #129426 has finished for PR 29947 at commit 109af99.

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

SparkQA · 2020-10-06T00:56:26Z

Test build #129427 has finished for PR 29947 at commit c683d78.

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

sql/core/src/test/resources/sql-tests/inputs/sql-compatibility-functions.sql

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

SparkQA · 2020-10-07T02:36:11Z

Test build #129465 has finished for PR 29947 at commit 52e16ec.

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

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

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2020-10-07T19:32:31Z

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

SparkQA · 2020-10-07T19:50:35Z

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

Signed-off-by: Karen Feng <karen.feng@databricks.com>

SparkQA · 2020-10-07T20:59:25Z

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

SparkQA · 2020-10-07T21:22:55Z

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

SparkQA · 2020-10-07T23:23:20Z

Test build #129526 has finished for PR 29947 at commit e5ad9e0.

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

SparkQA · 2020-10-08T00:56:15Z

Test build #129530 has finished for PR 29947 at commit e921f66.

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

HyukjinKwon

Otherwise LGTM

sql/core/src/test/scala/org/apache/spark/sql/ColumnExpressionSuite.scala

maropu · 2020-10-08T01:42:00Z

LGTM, too.

…uite.scala

HyukjinKwon · 2020-10-08T03:05:21Z

Merged to master.

SparkQA · 2020-10-08T03:49:59Z

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

SparkQA · 2020-10-08T04:08:14Z

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

zero323 · 2020-10-08T07:26:35Z

python/pyspark/sql/functions.pyi

@@ -137,6 +137,8 @@ def sha1(col: ColumnOrName) -> Column: ...
 def sha2(col: ColumnOrName, numBits: int) -> Column: ...
 def hash(*cols: ColumnOrName) -> Column: ...
 def xxhash64(*cols: ColumnOrName) -> Column: ...
+def assert_true(col: ColumnOrName, errMsg: Union[Column, str] = ...): ...


Two small notes (sorry for being late):

I think we should annotate return type for assert_true - it will type check because of implicit Any, but I think it is better to avoid such cases

def assert_true(col: ColumnOrName, errMsg: Union[Column, str] = ...) -> Column: ...

For def raise_error I'd use NoReturn:

from typing import NoReturn def raise_error(errMsg: Union[Column, str]) -> NoReturn: ...

@karenfeng Could you fix them above in followup?

For def raise_error I'd use NoReturn:

This might indicate intention here, though technically speaking it's still a Column, so

def raise_error(errMsg: Union[Column, str]) -> Column: ...

is still correct (and literal one). Do you have any thoughts about it @HyukjinKwon?

I think doing Column is fine.

zero323 · 2020-10-08T08:20:24Z

It seem like R functions haven't been included in NAMESPACE. Unless it is intentional they should be added around here

spark/R/pkg/NAMESPACE

Line 231 in 5effa8e

"ascii",

and here

spark/R/pkg/NAMESPACE

Line 363 in 5effa8e

"radians",

for assert_true and raise_error respectively.

zero323 · 2020-10-08T08:23:38Z

R/pkg/R/functions.R

+            jc <- if (is.null(errMsg)) {
+              callJStatic("org.apache.spark.sql.functions", "assert_true", x@jc)
+            } else {
+              if (is.character(errMsg) && length(errMsg) == 1) {


Shouldn't we throw an exception if length(errMsg) != 1? Just in case user does something like this?

> assert_true(column("foo"), c("foo", "bar")) Error in invokeJava(isStatic = TRUE, className, methodName, ...) : trying to get slot "jc" from an object of a basic class ("character") with no slots

i.e.

... } else { if (is.character(errMsg) { stopifnot(length(errMsg) == 1) ...

Yeah, more checks should be fine.

In practice we make this check anyway, so it is only a question if we do something about it.

HyukjinKwon · 2020-10-08T09:04:47Z

@zero323 it's my bad that I rushed. Can you make a quick followup if you're available? I think @karenfeng lives in US timezone and probably is sleeping :-)

zero323 · 2020-10-08T10:16:02Z

@HyukjinKwon I am at work right now, with only my phone, but I'll open a PR once I am back home, unless it is resolved by then.

HyukjinKwon · 2020-10-08T10:28:49Z

Sure, thanks!

…d SparkR ### What changes were proposed in this pull request? - Annotated return types of `assert_true` and `raise_error` as discussed [here](#29947 (review)). - Add `assert_true` and `raise_error` to SparkR NAMESPACE. - Validating message vector size in SparkR as discussed [here](#29947 (review)). ### Why are the changes needed? As discussed in review for #29947. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Existing tests. - Validation of annotations using MyPy Closes #29978 from zero323/SPARK-32793-FOLLOW-UP. Authored-by: zero323 <mszymkiewicz@gmail.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org>

karenfeng added 13 commits September 30, 2020 17:35

WIP

92397c2

Signed-off-by: Karen Feng <karen.feng@databricks.com>

WIP

387c30c

Signed-off-by: Karen Feng <karen.feng@databricks.com>

WIP

001ff9c

Signed-off-by: Karen Feng <karen.feng@databricks.com>

WIP

4adcd70

Signed-off-by: Karen Feng <karen.feng@databricks.com>

fix example

d997562

Signed-off-by: Karen Feng <karen.feng@databricks.com>

fix CodeGenerationSuite

9facc82

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Add Scala tests

e1cf7bb

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Add Python function

cb685c3

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Fix doctests, add python tests for raise_error

40b9ef9

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Add R fn

79b1428

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Add R tests

c187226

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Trigger tests

3bab293

Merge branch 'master' of https://github.com/apache/spark into spark-3…

df1fc36

…2793 Signed-off-by: Karen Feng <karen.feng@databricks.com>

karenfeng changed the title ~~[WIP][SPARK-32793][SQL] Add raise_error function, adds error message parameter to assert_true~~ [SPARK-32793][SQL] Add raise_error function, adds error message parameter to assert_true Oct 5, 2020

Fix python style

0ff60f4

Signed-off-by: Karen Feng <karen.feng@databricks.com>

karenfeng added 2 commits October 5, 2020 15:58

R style

109af99

Signed-off-by: Karen Feng <karen.feng@databricks.com>

add errMsg to generics.R

c683d78

Signed-off-by: Karen Feng <karen.feng@databricks.com>

maropu reviewed Oct 6, 2020

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/TypeCoercionSuite.scala Outdated Show resolved Hide resolved

maropu reviewed Oct 6, 2020

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/sql-compatibility-functions.sql Outdated Show resolved Hide resolved

maropu reviewed Oct 6, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated Show resolved Hide resolved

viirya reviewed Oct 7, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala Show resolved Hide resolved

viirya reviewed Oct 7, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala Show resolved Hide resolved

viirya reviewed Oct 7, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala Outdated Show resolved Hide resolved

Address comments

e5ad9e0

Signed-off-by: Karen Feng <karen.feng@databricks.com>

Wrap strings in lit()

e921f66

Signed-off-by: Karen Feng <karen.feng@databricks.com>

HyukjinKwon approved these changes Oct 8, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/ColumnExpressionSuite.scala Outdated Show resolved Hide resolved

viirya approved these changes Oct 8, 2020

View reviewed changes

Update sql/core/src/test/scala/org/apache/spark/sql/ColumnExpressionS…

5b93fd4

…uite.scala

HyukjinKwon closed this in 39510b0 Oct 8, 2020

zero323 reviewed Oct 8, 2020

View reviewed changes

zero323 mentioned this pull request Oct 8, 2020

[SPARK-32793][FOLLOW-UP] Minor corrections for PySpark annotations and SparkR #29978

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-32793][SQL] Add raise_error function, adds error message parameter to assert_true #29947

[SPARK-32793][SQL] Add raise_error function, adds error message parameter to assert_true #29947

karenfeng commented Oct 5, 2020

SparkQA commented Oct 5, 2020

SparkQA commented Oct 5, 2020

SparkQA commented Oct 5, 2020

SparkQA commented Oct 5, 2020

SparkQA commented Oct 5, 2020

SparkQA commented Oct 5, 2020

SparkQA commented Oct 5, 2020

SparkQA commented Oct 6, 2020

SparkQA commented Oct 6, 2020

SparkQA commented Oct 6, 2020

SparkQA commented Oct 7, 2020

SparkQA commented Oct 7, 2020

SparkQA commented Oct 7, 2020

SparkQA commented Oct 7, 2020

SparkQA commented Oct 7, 2020

SparkQA commented Oct 7, 2020

SparkQA commented Oct 8, 2020

HyukjinKwon left a comment

maropu commented Oct 8, 2020

HyukjinKwon commented Oct 8, 2020

SparkQA commented Oct 8, 2020

SparkQA commented Oct 8, 2020

zero323 Oct 8, 2020

maropu Oct 8, 2020

zero323 Oct 8, 2020

HyukjinKwon Oct 8, 2020

zero323 commented Oct 8, 2020

zero323 Oct 8, 2020 •

edited

Loading

HyukjinKwon Oct 8, 2020

zero323 Oct 8, 2020

HyukjinKwon commented Oct 8, 2020

zero323 commented Oct 8, 2020

HyukjinKwon commented Oct 8, 2020

[SPARK-32793][SQL] Add raise_error function, adds error message parameter to assert_true #29947

[SPARK-32793][SQL] Add raise_error function, adds error message parameter to assert_true #29947

Conversation

karenfeng commented Oct 5, 2020

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?

SparkQA commented Oct 5, 2020

SparkQA commented Oct 5, 2020

SparkQA commented Oct 5, 2020

SparkQA commented Oct 5, 2020

SparkQA commented Oct 5, 2020

SparkQA commented Oct 5, 2020

SparkQA commented Oct 5, 2020

SparkQA commented Oct 6, 2020

SparkQA commented Oct 6, 2020

SparkQA commented Oct 6, 2020

SparkQA commented Oct 7, 2020

SparkQA commented Oct 7, 2020

SparkQA commented Oct 7, 2020

SparkQA commented Oct 7, 2020

SparkQA commented Oct 7, 2020

SparkQA commented Oct 7, 2020

SparkQA commented Oct 8, 2020

HyukjinKwon left a comment

Choose a reason for hiding this comment

maropu commented Oct 8, 2020

HyukjinKwon commented Oct 8, 2020

SparkQA commented Oct 8, 2020

SparkQA commented Oct 8, 2020

zero323 Oct 8, 2020

Choose a reason for hiding this comment

maropu Oct 8, 2020

Choose a reason for hiding this comment

zero323 Oct 8, 2020

Choose a reason for hiding this comment

HyukjinKwon Oct 8, 2020

Choose a reason for hiding this comment

zero323 commented Oct 8, 2020

zero323 Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

HyukjinKwon Oct 8, 2020

Choose a reason for hiding this comment

zero323 Oct 8, 2020

Choose a reason for hiding this comment

HyukjinKwon commented Oct 8, 2020

zero323 commented Oct 8, 2020

HyukjinKwon commented Oct 8, 2020

zero323 Oct 8, 2020 •

edited

Loading