[SPARK-32870][DOCS][SQL] Make sure that all expressions have their ExpressionDescription filled #29743

tanelk · 2020-09-13T20:48:26Z

What changes were proposed in this pull request?

Made sure, that all the expressions in the FunctionRegistry have the fields usage, examples and since filled in their ExpressionDescription. Added UT to ExpressionInfoSuite, to make sure, that all new expressions will also fill those fields.

Why are the changes needed?

Documentation improvement

Does this PR introduce any user-facing change?

Better generated SQL built in functions documentation

How was this patch tested?

Checked the fix version in the following jiras:
SPARK-1251 - UnaryMinus, Add, Subtract, Multiply, Divide, Remainder, Explode, Not, In, And, Or, Equals, LessThan, LessThanOrEqual, GreaterThan, GreaterThanOrEqual, If, Cast
SPARK-2053 - CaseWhen
SPARK-2665 - EqualNullSafe
SPARK-3176 - Abs
SPARK-6542 - CreateStruct
SPARK-7135 - MonotonicallyIncreasingID
SPARK-7152 - SparkPartitionID
SPARK-7295 - bitwiseAND, bitwiseOR, bitwiseXOR, bitwiseNOT
SPARK-8005 - InputFileName
SPARK-8203 - Greatest
SPARK-8204 - Least
SPARK-8220 - UnaryPositive
SPARK-8221 - Pmod
SPARK-8230 - Size
SPARK-8231 - ArrayContains
SPARK-8232 - SortArray
SPARK-8234 - md5
SPARK-8235 - sha1
SPARK-8236 - crc32
SPARK-8237 - sha2
SPARK-8240 - Concat
SPARK-8246 - GetJsonObject
SPARK-8407 - CreateNamedStruct
SPARK-9617 - JsonTuple
SPARK-10810 - CurrentDatabase
SPARK-12480 - Murmur3Hash
SPARK-14061 - CreateMap
SPARK-14160 - TimeWindow
SPARK-14580 - AssertTrue
SPARK-16274 - XPathBoolean
SPARK-16278 - MapKeys
SPARK-16279 - MapValues
SPARK-16284 - CallMethodViaReflection
SPARK-16286 - Stack
SPARK-16288 - Inline
SPARK-16289 - PosExplode
SPARK-16318 - XPathShort, XPathInt, XPathLong, XPathFloat, XPathDouble, XPathString, XPathList
SPARK-16730 - Cast aliases
SPARK-17495 - HiveHash
SPARK-18702 - InputFileBlockStart, InputFileBlockLength
SPARK-20910 - UUID

…rly filled

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

...t/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/CountMinSketchAgg.scala

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

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

tanelk · 2020-09-13T20:59:43Z

cc @maropu and @HyukjinKwon , you reviewed the previous similar pull request

HyukjinKwon · 2020-09-13T23:49:27Z

ok to test

HyukjinKwon · 2020-09-13T23:49:56Z

Looks there are some tests failed at https://github.com/apache/spark/pull/29743/checks?check_run_id=1109584652.

HyukjinKwon · 2020-09-13T23:50:15Z

Thanks for working on this, @tanelk.

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

SparkQA · 2020-09-14T01:59:33Z

Test build #128610 has finished for PR 29743 at commit c7488e0.

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

SparkQA · 2020-09-14T10:05:25Z

Test build #128629 has finished for PR 29743 at commit 9b9bfca.

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

SparkQA · 2020-09-14T10:30:21Z

Test build #128636 has finished for PR 29743 at commit 802f338.

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

SparkQA · 2020-09-14T10:33:24Z

Test build #128640 has finished for PR 29743 at commit b67c967.

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

maropu

Thanks for the hard work, @tanelk! This update looks very useful. I've checked them and the versions I think you investigated in this PR looks correct.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala

...yst/src/main/scala/org/apache/spark/sql/catalyst/expressions/MonotonicallyIncreasingID.scala

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

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

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

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

sql/core/src/test/scala/org/apache/spark/sql/expressions/ExpressionInfoSuite.scala

SparkQA · 2020-09-14T14:25:27Z

Test build #128646 has finished for PR 29743 at commit 98287e4.

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

SparkQA · 2020-09-14T15:10:20Z

Test build #128641 has finished for PR 29743 at commit 930394c.

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

SparkQA · 2020-09-14T15:37:40Z

Test build #128644 has finished for PR 29743 at commit 4e9291e.

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

SparkQA · 2020-09-14T19:08:43Z

Test build #128655 has finished for PR 29743 at commit 3ba2515.

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

sql/core/src/test/scala/org/apache/spark/sql/expressions/ExpressionInfoSuite.scala

maropu · 2020-09-15T00:13:56Z

sql/core/src/test/scala/org/apache/spark/sql/expressions/ExpressionInfoSuite.scala

+          assert(info.getUsage.nonEmpty)
+          assert(info.getExamples.startsWith("\n    Examples:\n"))
+          assert(info.getExamples.endsWith("\n  "))
+          assert(info.getSince.matches("[0-9]+\\.[0-9]+\\.[0-9]+"))


Looks these checks have already been done in the ExpressionInfo constructor?

These ones are a bit more strict.
For example this extra space made the Example for typeof not appear in http://spark.apache.org/docs/latest/api/sql/#typeof

https://github.com/apache/spark/pull/29743/files#diff-25282ab1377a3d87999d1d0d7a8ec270R208-R214

I didn't want to add these checks to the constructor, because I'm afraid that they might break some UDFs and also I have no way to exclude some from the checks.

I didn't want to add these checks to the constructor, because I'm afraid that they might break some UDFs and also I have no way to exclude some from the checks.

If we make the checks more strict in the ctor and we find existing expressions throwing exceptions, I think it is okay just to fix them. Any problem there?

No, problems with current ones - only some dummy functions in tests.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala

SparkQA · 2020-09-15T07:05:01Z

Test build #128692 has finished for PR 29743 at commit 499c32c.

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

SparkQA · 2020-09-15T07:05:01Z

Test build #128697 has finished for PR 29743 at commit 2bcc663.

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

# Conflicts: # sql/core/src/test/resources/sql-functions/sql-expression-schema.md

SparkQA · 2020-09-19T23:30:11Z

Test build #128892 has finished for PR 29743 at commit 4162b41.

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

HyukjinKwon

Looks good but I will leave it to @maropu.

maropu · 2020-09-23T01:19:00Z

Thanks! Merged to master.

…pressionDescription filled ### What changes were proposed in this pull request? Made sure, that all the expressions in the `FunctionRegistry ` have the fields `usage`, `examples` and `since` filled in their `ExpressionDescription`. Added UT to `ExpressionInfoSuite`, to make sure, that all new expressions will also fill those fields. ### Why are the changes needed? Documentation improvement ### Does this PR introduce _any_ user-facing change? Better generated SQL built in functions documentation ### How was this patch tested? Checked the fix version in the following jiras: SPARK-1251 - UnaryMinus, Add, Subtract, Multiply, Divide, Remainder, Explode, Not, In, And, Or, Equals, LessThan, LessThanOrEqual, GreaterThan, GreaterThanOrEqual, If, Cast SPARK-2053 - CaseWhen SPARK-2665 - EqualNullSafe SPARK-3176 - Abs SPARK-6542 - CreateStruct SPARK-7135 - MonotonicallyIncreasingID SPARK-7152 - SparkPartitionID SPARK-7295 - bitwiseAND, bitwiseOR, bitwiseXOR, bitwiseNOT SPARK-8005 - InputFileName SPARK-8203 - Greatest SPARK-8204 - Least SPARK-8220 - UnaryPositive SPARK-8221 - Pmod SPARK-8230 - Size SPARK-8231 - ArrayContains SPARK-8232 - SortArray SPARK-8234 - md5 SPARK-8235 - sha1 SPARK-8236 - crc32 SPARK-8237 - sha2 SPARK-8240 - Concat SPARK-8246 - GetJsonObject SPARK-8407 - CreateNamedStruct SPARK-9617 - JsonTuple SPARK-10810 - CurrentDatabase SPARK-12480 - Murmur3Hash SPARK-14061 - CreateMap SPARK-14160 - TimeWindow SPARK-14580 - AssertTrue SPARK-16274 - XPathBoolean SPARK-16278 - MapKeys SPARK-16279 - MapValues SPARK-16284 - CallMethodViaReflection SPARK-16286 - Stack SPARK-16288 - Inline SPARK-16289 - PosExplode SPARK-16318 - XPathShort, XPathInt, XPathLong, XPathFloat, XPathDouble, XPathString, XPathList SPARK-16730 - Cast aliases SPARK-17495 - HiveHash SPARK-18702 - InputFileBlockStart, InputFileBlockLength SPARK-20910 - UUID Closes #29743 from tanelk/SPARK-32870. Authored-by: [email protected] <[email protected]> Signed-off-by: Takeshi Yamamuro <[email protected]>

Make sure that all expressions have their ExpressionDescription prope…

533c95f

…rly filled

probot-autolabeler bot added the SQL label Sep 13, 2020

tanelk commented Sep 13, 2020

View reviewed changes

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

tanelk commented Sep 13, 2020

View reviewed changes

...t/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/CountMinSketchAgg.scala Show resolved Hide resolved

tanelk commented Sep 13, 2020

View reviewed changes

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

tanelk commented Sep 13, 2020

View reviewed changes

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

tanelk added 2 commits September 14, 2020 00:49

Move the assertions to UT

ef592e7

Style fix

cdbff09

tanelk changed the title ~~[WIP][SPARK-32870][DOCS][SQL] Make sure that all expressions have their ExpressionDescription filled~~ [SPARK-32870][DOCS][SQL] Make sure that all expressions have their ExpressionDescription filled Sep 13, 2020

Regenerate golden files

20ba238

Update UT

c7488e0

HyukjinKwon reviewed Sep 13, 2020

View reviewed changes

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

tanelk added 3 commits September 14, 2020 10:07

Improve some examples

d3f9aad

Fix a failing test

9b9bfca

Make sure that Arguments and Examples have a correct format

802f338

Fix a failing test

b67c967

tanelk added 3 commits September 14, 2020 13:33

Fix a failing test

930394c

Makse sure that examples have the correct format

4e9291e

Remove the documentation from TimeWindow

98287e4

maropu reviewed Sep 14, 2020

View reviewed changes

Adress comments

3ba2515

maropu reviewed Sep 14, 2020

View reviewed changes

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

maropu reviewed Sep 15, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala Outdated Show resolved Hide resolved

tanelk added 2 commits September 15, 2020 06:37

Address comments

499c32c

Fix a failing test

2bcc663

tanelk added 2 commits September 19, 2020 21:35

Merge branch 'master' into SPARK-32870

12af9cd

# Conflicts: # sql/core/src/test/resources/sql-functions/sql-expression-schema.md

Fill in the examples for NthValue

4162b41

tanelk requested review from HyukjinKwon and maropu September 21, 2020 17:35

HyukjinKwon approved these changes Sep 22, 2020

View reviewed changes

maropu approved these changes Sep 23, 2020

View reviewed changes

maropu closed this Sep 23, 2020

[SPARK-32870][DOCS][SQL] Make sure that all expressions have their ExpressionDescription filled #29743

[SPARK-32870][DOCS][SQL] Make sure that all expressions have their ExpressionDescription filled #29743

Uh oh!

Conversation

tanelk commented Sep 13, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tanelk commented Sep 13, 2020

Uh oh!

HyukjinKwon commented Sep 13, 2020

Uh oh!

HyukjinKwon commented Sep 13, 2020

Uh oh!

HyukjinKwon commented Sep 13, 2020

Uh oh!

Uh oh!

SparkQA commented Sep 14, 2020

Uh oh!

SparkQA commented Sep 14, 2020

Uh oh!

SparkQA commented Sep 14, 2020

Uh oh!

SparkQA commented Sep 14, 2020

Uh oh!

maropu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Sep 14, 2020

Uh oh!

SparkQA commented Sep 14, 2020

Uh oh!

SparkQA commented Sep 14, 2020

Uh oh!

SparkQA commented Sep 14, 2020

Uh oh!

Uh oh!

maropu Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

tanelk Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

maropu Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

tanelk Sep 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Sep 15, 2020

Uh oh!

SparkQA commented Sep 15, 2020

Uh oh!

SparkQA commented Sep 19, 2020

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

maropu commented Sep 23, 2020

Uh oh!

Reviewers

Assignees

tanelk commented Sep 13, 2020 •

edited

Loading