[SPARK-23327] [SQL] Update the description and tests of three external API or functions #20495

gatorsmile · 2018-02-03T07:33:28Z

What changes were proposed in this pull request?

Update the description and tests of three external API or functions createFunction , length and repartitionByRange

How was this patch tested?

N/A

gatorsmile · 2018-02-03T07:33:42Z

cc @srinathshankar @rxin @cloud-fan

SparkQA · 2018-02-03T07:45:43Z

Test build #87026 has finished for PR 20495 at commit 9ecc809.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-02-03T11:11:20Z

Test build #87027 has finished for PR 20495 at commit c33cc9a.

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

dongjoon-hyun · 2018-02-03T16:55:38Z

python/pyspark/sql/functions.py


-    >>> spark.createDataFrame([('ABC',)], ['a']).select(length('a').alias('length')).collect()
-    [Row(length=3)]
+    >>> spark.createDataFrame([('ABC ',)], ['a']).select(length('a').alias('length')).collect()


Actually, not only description, this PR improves the test coverage and refactos the code, too.
Could you update the PR description/title more correctly?
Otherwise, we had better split this PR according to @rdblue 's recommendations in our dev mailing list.

dongjoon-hyun

+1, LGTM.

felixcheung · 2018-02-04T19:36:13Z

python/pyspark/sql/functions.py

@@ -1705,10 +1705,12 @@ def unhex(col):
 @ignore_unicode_prefix
 @since(1.5)
 def length(col):
-    """Calculates the length of a string or binary expression.
+    """Computes the character length of a given string or number of bytes or a binary string.
+    The length of character strings include the trailing spaces. The length of binary strings


as a side note, why is it calling out trailing spaces? what about leading spaces? isn't all spaces factored into the character length?

I ask because I want to understand this better to see if we should update R https://github.com/apache/spark/blob/master/R/pkg/R/functions.R#L1029

The reason is LEN in MS SQL Server excludes trailing blanks. : )

Yeah. This PR also can updates it in R side too.

cloud-fan · 2018-02-05T10:12:38Z

python/pyspark/sql/functions.py

@@ -1705,10 +1705,12 @@ def unhex(col):
 @ignore_unicode_prefix
 @since(1.5)
 def length(col):
-    """Calculates the length of a string or binary expression.
+    """Computes the character length of a given string or number of bytes or a binary string.


number of bytes of a binary value?

SparkQA · 2018-02-06T02:29:31Z

Test build #87081 has finished for PR 20495 at commit 9e97db9.

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

cloud-fan · 2018-02-06T02:39:57Z

retest this please

cloud-fan · 2018-02-06T02:42:12Z

LGTM

SparkQA · 2018-02-06T05:10:31Z

Test build #87085 has finished for PR 20495 at commit 9e97db9.

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

cloud-fan · 2018-02-06T05:19:41Z

retest this please

felixcheung

LG, one comment

felixcheung · 2018-02-06T06:29:49Z

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

@@ -1655,15 +1655,17 @@ case class Left(str: Expression, len: Expression, child: Expression) extends Run
 */
 // scalastyle:off line.size.limit
 @ExpressionDescription(
-  usage = "_FUNC_(expr) - Returns the character length of `expr` or number of bytes in binary data.",
+  usage = "_FUNC_(expr) - Returns the character length of `expr` or number of bytes in binary data. " +


why are other places use "binary string" and here we have "binary data"?

We should be consistent, either character string vs binary string, or string data vs binary data.

+1 for string data / binary data

SparkQA · 2018-02-06T07:51:13Z

Test build #87091 has finished for PR 20495 at commit 9e97db9.

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

SparkQA · 2018-02-06T22:54:34Z

Test build #87124 has finished for PR 20495 at commit b837053.

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

… API or functions ## What changes were proposed in this pull request? Update the description and tests of three external API or functions `createFunction `, `length` and `repartitionByRange ` ## How was this patch tested? N/A Author: gatorsmile <[email protected]> Closes #20495 from gatorsmile/updateFunc. (cherry picked from commit c36fecc) Signed-off-by: gatorsmile <[email protected]>

gatorsmile · 2018-02-07T21:29:54Z

Thanks! Merged to master/2.3

… API or functions ## What changes were proposed in this pull request? Update the description and tests of three external API or functions `createFunction `, `length` and `repartitionByRange ` ## How was this patch tested? N/A Author: gatorsmile <[email protected]> Closes apache#20495 from gatorsmile/updateFunc.

gatorsmile added 3 commits February 2, 2018 20:35

update create function.

48f552c

update the comment for repartitionByRange

21ea233

update the comment of length

9ecc809

fix the test

c33cc9a

dongjoon-hyun reviewed Feb 3, 2018

View reviewed changes

gatorsmile changed the title ~~[SPARK-23327] [SQL] Update the description of three external API or functions~~ [SPARK-23327] [SQL] Update the description and tests of three external API or functions Feb 3, 2018

dongjoon-hyun approved these changes Feb 4, 2018

View reviewed changes

felixcheung approved these changes Feb 4, 2018

View reviewed changes

cloud-fan reviewed Feb 5, 2018

View reviewed changes

address comments.

9e97db9

felixcheung approved these changes Feb 6, 2018

View reviewed changes

address comments.

b837053

asfgit closed this in c36fecc Feb 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-23327] [SQL] Update the description and tests of three external API or functions #20495

[SPARK-23327] [SQL] Update the description and tests of three external API or functions #20495

gatorsmile commented Feb 3, 2018 •

edited

Loading

gatorsmile commented Feb 3, 2018

SparkQA commented Feb 3, 2018

SparkQA commented Feb 3, 2018

dongjoon-hyun Feb 3, 2018 •

edited

Loading

gatorsmile Feb 3, 2018

dongjoon-hyun left a comment

felixcheung Feb 4, 2018

felixcheung Feb 4, 2018

gatorsmile Feb 4, 2018

cloud-fan Feb 5, 2018

SparkQA commented Feb 6, 2018

cloud-fan commented Feb 6, 2018

cloud-fan commented Feb 6, 2018

SparkQA commented Feb 6, 2018

cloud-fan commented Feb 6, 2018

felixcheung left a comment

felixcheung Feb 6, 2018

cloud-fan Feb 6, 2018

rdblue Feb 6, 2018

SparkQA commented Feb 6, 2018

SparkQA commented Feb 6, 2018

gatorsmile commented Feb 7, 2018

[SPARK-23327] [SQL] Update the description and tests of three external API or functions #20495

[SPARK-23327] [SQL] Update the description and tests of three external API or functions #20495

Conversation

gatorsmile commented Feb 3, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

gatorsmile commented Feb 3, 2018

SparkQA commented Feb 3, 2018

SparkQA commented Feb 3, 2018

dongjoon-hyun Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

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

Choose a reason for hiding this comment

SparkQA commented Feb 6, 2018

cloud-fan commented Feb 6, 2018

cloud-fan commented Feb 6, 2018

SparkQA commented Feb 6, 2018

cloud-fan commented Feb 6, 2018

felixcheung left a comment

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 Feb 6, 2018

SparkQA commented Feb 6, 2018

gatorsmile commented Feb 7, 2018

gatorsmile commented Feb 3, 2018 •

edited

Loading

dongjoon-hyun Feb 3, 2018 •

edited

Loading