[SPARK-33527][SQL] Extend the function of decode so as consistent with mainstream databases #30479

beliefer · 2020-11-24T07:47:55Z

What changes were proposed in this pull request?

In Spark, decode(bin, charset) - Decodes the first argument using the second argument character set.

Unfortunately this is NOT what any other SQL vendor understands DECODE to do.
DECODE generally is a short hand for a simple case expression:

SELECT DECODE(c1, 1, 'Hello', 2, 'World', '!') FROM (VALUES (1), (2), (3)) AS T(c1)
=> 
(Hello),
(World)
(!)

There are some mainstream database support the syntax.
Oracle
https://docs.oracle.com/en/database/oracle/oracle-database/19/sqlrf/DECODE.html#GUID-39341D91-3442-4730-BD34-D3CF5D4701CE
Vertica
https://www.vertica.com/docs/9.2.x/HTML/Content/Authoring/SQLReferenceManual/Functions/String/DECODE.htm?tocpath=SQL%20Reference%20Manual%7CSQL%20Functions%7CString%20Functions%7C_____10
DB2
https://www.ibm.com/support/knowledgecenter/SSGU8G_14.1.0/com.ibm.sqls.doc/ids_sqs_1447.htm
Redshift
https://docs.aws.amazon.com/redshift/latest/dg/r_DECODE_expression.html
Pig
https://pig.apache.org/docs/latest/api/org/apache/pig/piggybank/evaluation/decode/Decode.html
Teradata
https://docs.teradata.com/reader/756LNiPSFdY~4JcCCcR5Cw/jtCpCycpEaXESG4d63kMjg
Snowflake
https://docs.snowflake.com/en/sql-reference/functions/decode.html

Why are the changes needed?

It is very useful.

Does this PR introduce any user-facing change?

Yes

How was this patch tested?

Jenkins test.

SparkQA · 2020-12-08T10:24:15Z

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

SparkQA · 2020-12-08T10:42:19Z

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

SparkQA · 2020-12-08T10:52:46Z

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

SparkQA · 2020-12-08T11:17:56Z

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

SparkQA · 2020-12-08T12:26:02Z

Test build #132427 has finished for PR 30479 at commit 6a1b919.

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

SparkQA · 2020-12-08T12:56:32Z

Test build #132426 has finished for PR 30479 at commit 80c56a1.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class Decode(params: Seq[Expression], child: Expression) extends RuntimeReplaceable

cloud-fan · 2020-12-08T14:16:06Z

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

+  def createExpr(params: Seq[Expression]): Expression = {
+    params.length match {
+      case 0 => StringDecode(Literal.create(null, StringType), Literal.create(null, StringType))
+      case 1 => StringDecode(params.head, Literal.create(null, StringType))


do we really allow 0 or 1 parameter before?

Let's preserve the AnalysisException(s"Invalid number of arguments for function decode. ")

SparkQA · 2020-12-09T05:34:05Z

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

SparkQA · 2020-12-09T06:05:00Z

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

maropu · 2020-12-09T07:05:18Z

Just a question; the other systems don't have a function like Spark decode/encode?

maropu · 2020-12-09T07:12:16Z

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

+      case 0 | 1 =>
+        throw new AnalysisException("Invalid number of arguments for function decode. " +
+          s"Expected: 2; Found: ${params.length}")
+      case 2 => StringDecode(params.head, params.last)


hm, IMHO it would be good to rename the existing decode func or assign a new name into the new decode func because the generated doc for decode will be complicated.

If rename the existing decode func will break the behavior.

Yea, so I think we need add a legacy config, update the migration guide, ... if we rename it. Anyway, the latter approach (assigning a new name into the new decode func) looks better if possible.

This PR could avoid migration and support the new decode as well. cc @cloud-fan

We can probably rename the function in ansi mode.

maropu · 2020-12-09T07:16:22Z

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

+            |_FUNC_(expr, search, result [, search, result ] ... [, default]) - Decode compares expr
+            |  to each search value one by one. If expr is equal to a search, returns the corresponding result.
+            |  If no match is found, then Oracle returns default. If default is omitted, returns null.
+          """,


Could you follow the other format? e.g.,

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

Lines 479 to 486 in c88edda

usage = "_FUNC_(str, search[, replace]) - Replaces all occurrences of `search` with `replace`.",

arguments = """

Arguments:

* str - a string expression

* search - a string expression. If `search` is not found in `str`, `str` is returned unchanged.

* replace - a string expression. If `replace` is not specified or is an empty string, nothing replaces

the string that is removed from `str`.

""",

The new decode function is the same as other function uses RuntimeReplaceable. The parameter list is not fixed.

beliefer · 2020-12-09T08:37:24Z

Just a question; the other systems don't have a function like Spark decode/encode?

Only PostgreSQL have the function.

HyukjinKwon · 2020-12-09T08:54:16Z

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

+      > SELECT _FUNC_(6, 1, 'Southlake', 2, 'San Francisco', 3, 'New Jersey', 4, 'Seattle');
+       NULL
+  """,
+  since = "3.1.0")


BTW, new feature should arrive in 3.2.0 because branch-3.1 is out now.

SparkQA · 2020-12-09T09:00:01Z

Test build #132460 has finished for PR 30479 at commit 1614e62.

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

SparkQA · 2020-12-09T10:26:18Z

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

SparkQA · 2020-12-09T10:54:57Z

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

SparkQA · 2020-12-09T14:16:25Z

Test build #132484 has finished for PR 30479 at commit eea967a.

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

cloud-fan · 2020-12-11T05:52:32Z

thanks, merging to master!

HyukjinKwon · 2020-12-11T05:55:37Z

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

+  """,
+  since = "3.2.0")
+// scalastyle:on line.size.limit
+case class Decode(params: Seq[Expression], child: Expression) extends RuntimeReplaceable {


So this replaces the existing decode in SQL side only, right? shall we update migration guide?

The new decode could keep compatible with old one.

HyukjinKwon · 2020-12-11T05:56:50Z

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

  since = "1.5.0")
 // scalastyle:on line.size.limit
-case class Decode(bin: Expression, charset: Expression)
+case class StringDecode(bin: Expression, charset: Expression)


Do we have an alternative for the previous decode after this change?

This only renames the expression, not the function. This PR just adds an overload of the decode function when there are more than 3 parameters.

I'd suggest the following followup:

add a new function name for StringDecode. Probably "string_decode".

require decode function to take more than 2 parameters, under ansi mode(or a legacy config)

beliefer · 2020-12-11T06:10:23Z

@cloud-fan Thanks for your work
@maropu @HyukjinKwon Thanks for your review too.

### What changes were proposed in this pull request? This PR fixes the Scala 2.13 build failure brought by #30479 . ### Why are the changes needed? To pass Scala 2.13 build. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Should be done byGitHub Actions. Closes #30727 from sarutak/fix-scala213-build-failure. Authored-by: Kousuke Saruta <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

beliefer and others added 30 commits June 19, 2020 10:36

Reuse completeNextStageWithFetchFailure

4a6f903

Merge remote-tracking branch 'upstream/master'

96456e2

Merge remote-tracking branch 'upstream/master'

4314005

Merge remote-tracking branch 'upstream/master'

d6af4a7

Merge remote-tracking branch 'upstream/master'

f69094f

Merge remote-tracking branch 'upstream/master'

b86a42d

Merge branch 'master' of github.com:beliefer/spark

2ac5159

Merge remote-tracking branch 'upstream/master'

9021d6c

Merge branch 'master' of github.com:beliefer/spark

74a2ef4

Merge remote-tracking branch 'upstream/master'

9828158

Merge remote-tracking branch 'upstream/master'

9cd1aaf

Merge remote-tracking branch 'upstream/master'

abfcbb9

Merge remote-tracking branch 'upstream/master'

07c6c81

Merge remote-tracking branch 'upstream/master'

580130b

Merge branch 'master' of github.com:beliefer/spark

3712808

Merge remote-tracking branch 'upstream/master'

6107413

Merge remote-tracking branch 'upstream/master'

4b799b4

Merge remote-tracking branch 'upstream/master'

ee0ecbf

Merge remote-tracking branch 'upstream/master'

596bc61

Merge remote-tracking branch 'upstream/master'

0164e2f

Merge remote-tracking branch 'upstream/master'

90b79fc

Merge remote-tracking branch 'upstream/master'

2cef3a9

Merge remote-tracking branch 'upstream/master'

c26b64f

Merge remote-tracking branch 'upstream/master'

2e02cd2

Merge remote-tracking branch 'upstream/master'

a6d0741

Merge remote-tracking branch 'upstream/master'

82e5b2c

Merge remote-tracking branch 'upstream/master'

70bbf5d

Merge remote-tracking branch 'upstream/master'

126a51e

Merge remote-tracking branch 'upstream/master'

f2ceacd

Merge remote-tracking branch 'upstream/master'

5ad208f

beliefer added 2 commits December 8, 2020 17:33

Optimize code

80c56a1

Update comments

6a1b919

cloud-fan reviewed Dec 8, 2020

View reviewed changes

Optimize code

1614e62

maropu reviewed Dec 9, 2020

View reviewed changes

HyukjinKwon reviewed Dec 9, 2020

View reviewed changes

Update since

eea967a

cloud-fan approved these changes Dec 11, 2020

View reviewed changes

cloud-fan closed this in 24d7e45 Dec 11, 2020

HyukjinKwon reviewed Dec 11, 2020

View reviewed changes

sarutak mentioned this pull request Dec 11, 2020

[SPARK-33527][SQL][FOLLOWUP] Fix the scala 2.13 build failure #30727

Closed

	usage = "_FUNC_(str, search[, replace]) - Replaces all occurrences of `search` with `replace`.",
	arguments = """
	Arguments:
	* str - a string expression
	* search - a string expression. If `search` is not found in `str`, `str` is returned unchanged.
	* replace - a string expression. If `replace` is not specified or is an empty string, nothing replaces
	the string that is removed from `str`.
	""",

[SPARK-33527][SQL] Extend the function of decode so as consistent with mainstream databases #30479

[SPARK-33527][SQL] Extend the function of decode so as consistent with mainstream databases #30479

Uh oh!

Conversation

beliefer commented Nov 24, 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 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

SparkQA commented Dec 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 9, 2020

Uh oh!

SparkQA commented Dec 9, 2020

Uh oh!

maropu commented Dec 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer commented Dec 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 9, 2020

Uh oh!

SparkQA commented Dec 9, 2020

Uh oh!

SparkQA commented Dec 9, 2020

Uh oh!

SparkQA commented Dec 9, 2020

Uh oh!

cloud-fan commented Dec 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beliefer commented Dec 11, 2020

Uh oh!

Reviewers

Assignees

Labels

beliefer commented Nov 24, 2020 •

edited

Loading

beliefer Dec 11, 2020 •

edited

Loading