[SPARK-29237][SQL] Prevent real function names in expression example template #25924

MaxGekk · 2019-09-25T06:43:08Z

What changes were proposed in this pull request?

In the PR, I propose to replace function names in some expression examples by _FUNC_, and add a test to check that _FUNC_ always present in all examples.

Why are the changes needed?

Binding of a function name to an expression is performed in FunctionRegistry which is single source of truth. Expression examples should avoid using function name directly because this can make the examples invalid in the future.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added new test to SQLQuerySuite which analyses expression example, and check presence of _FUNC_.

MaxGekk · 2019-09-25T06:48:37Z

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

@@ -1732,9 +1732,9 @@ case class Left(str: Expression, len: Expression, child: Expression) extends Run
    Examples:
      > SELECT _FUNC_('Spark SQL ');
       10
-      > SELECT CHAR_LENGTH('Spark SQL ');
+      > SELECT _FUNC_('Spark SQL ');


Would it be better show examples for binary data instead of demonstrating synonyms of the function name?

And it would be nice to show to users all synonyms in a separate list somewhere in the description

https://issues.apache.org/jira/browse/SPARK-29241

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

SparkQA · 2019-09-25T07:05:01Z

Test build #111331 has finished for PR 25924 at commit 565368d.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

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

wangyum · 2019-09-25T08:35:32Z

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

-      > SELECT CHARACTER_LENGTH('Spark SQL ');
+      > SELECT _FUNC_('Spark SQL');
+       9
+      > SELECT _FUNC_(binary('Spark SQL\\000'));


The last two examples seems to show to users we also support CHAR_LENGTH and CHARACTER_LENGTH :
https://issues.apache.org/jira/browse/SPARK-20749

ok. I will revert this but #25924 (comment) instead of hard coded names in examples.

SparkQA · 2019-09-25T10:35:13Z

Test build #111334 has finished for PR 25924 at commit f7caecd.

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

srowen · 2019-09-25T11:24:29Z

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/ExpressionInfo.java

@@ -56,6 +56,8 @@ public String getArguments() {
        return arguments;
    }

+    public String getOriginalExamples() { return examples; }


I don't know that this is worth adding just for testing?

+1 for @srowen 's opinion.

@MaxGekk . Instead of adding this API, shall we use the following format in the test case?

val examplesField = classOf[ExpressionInfo].getDeclaredField("examples") examplesField.setAccessible(true) val examples = examplesField.get(e).asInstanceOf[String]

Or can we use @VisibleForTesting at least?

SparkQA · 2019-09-25T11:48:21Z

Test build #111336 has finished for PR 25924 at commit b722806.

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

dongjoon-hyun · 2019-09-25T11:59:30Z

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

+      "org.apache.spark.sql.catalyst.expressions.Remainder",
+      // Examples demonstrate alternative names, see SPARK-20749
+      "org.apache.spark.sql.catalyst.expressions.Length")
+    spark.sessionState.functionRegistry.listFunction().foreach { funcId =>


Thank you, @MaxGekk . This is super useful.

SparkQA · 2019-09-25T12:11:43Z

Test build #111342 has finished for PR 25924 at commit aaa003e.

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

MaxGekk · 2019-09-25T12:13:37Z

I do believe it is worth because it is added not only for particular test, this check will prevent mistakes in docs.

SparkQA · 2019-09-25T16:12:52Z

Test build #111352 has finished for PR 25924 at commit 62eaea9.

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

MaxGekk · 2019-09-25T16:54:37Z

jenkins, retest this, please

SparkQA · 2019-09-25T20:21:59Z

Test build #111362 has finished for PR 25924 at commit 62eaea9.

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

dongjoon-hyun

+1, LGTM. Merged to master.
Thank you, @MaxGekk , @wangyum , @srowen !

MaxGekk added 3 commits September 25, 2019 09:29

Add a test

9e835a3

Expose original examples from ExpressionInfo

9ffa734

Fix examples

565368d

MaxGekk commented Sep 25, 2019

View reviewed changes

wangyum reviewed Sep 25, 2019

View reviewed changes

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

MaxGekk added 2 commits September 25, 2019 10:32

Add StringLocate to the ignore set

c9e8a0c

Replace by examples showing corner cases

f7caecd

MaxGekk commented Sep 25, 2019

View reviewed changes

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

Ignore examples for Remainder

b722806

wangyum reviewed Sep 25, 2019

View reviewed changes

Ignore examples for Length

aaa003e

srowen reviewed Sep 25, 2019

View reviewed changes

dongjoon-hyun added the SQL label Sep 25, 2019

dongjoon-hyun reviewed Sep 25, 2019

View reviewed changes

Add @VisibleForTesting to getOriginalExamples()

62eaea9

dongjoon-hyun changed the title ~~[SPARK-29237][SQL][DOC] Avoid usage of function names in expression examples~~ [SPARK-29237][SQL] Prevent real function names in expression example template Sep 25, 2019

dongjoon-hyun approved these changes Sep 25, 2019

View reviewed changes

dongjoon-hyun closed this in 21db2f8 Sep 25, 2019

MaxGekk deleted the fix-func-examples branch October 5, 2019 19:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-29237][SQL] Prevent real function names in expression example template #25924

[SPARK-29237][SQL] Prevent real function names in expression example template #25924

MaxGekk commented Sep 25, 2019

MaxGekk Sep 25, 2019

MaxGekk Sep 25, 2019

wangyum Sep 25, 2019

MaxGekk Sep 25, 2019

SparkQA commented Sep 25, 2019

wangyum Sep 25, 2019

MaxGekk Sep 25, 2019

SparkQA commented Sep 25, 2019

srowen Sep 25, 2019

dongjoon-hyun Sep 25, 2019 •

edited

srowen Sep 25, 2019

SparkQA commented Sep 25, 2019

dongjoon-hyun Sep 25, 2019

SparkQA commented Sep 25, 2019

MaxGekk commented Sep 25, 2019 via email •

edited

SparkQA commented Sep 25, 2019

MaxGekk commented Sep 25, 2019

SparkQA commented Sep 25, 2019

dongjoon-hyun left a comment

[SPARK-29237][SQL] Prevent real function names in expression example template #25924

[SPARK-29237][SQL] Prevent real function names in expression example template #25924

Conversation

MaxGekk commented Sep 25, 2019

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?

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

SparkQA commented Sep 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Sep 25, 2019

Choose a reason for hiding this comment

dongjoon-hyun Sep 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Sep 25, 2019

Choose a reason for hiding this comment

SparkQA commented Sep 25, 2019

MaxGekk commented Sep 25, 2019 via email • edited

SparkQA commented Sep 25, 2019

MaxGekk commented Sep 25, 2019

SparkQA commented Sep 25, 2019

dongjoon-hyun left a comment

Choose a reason for hiding this comment

dongjoon-hyun Sep 25, 2019 •

edited

MaxGekk commented Sep 25, 2019 via email •

edited