[SPARK-29776][SQL] rpad and lpad should return NULL when padstring parameter is empty #26477

07ARB · 2019-11-12T05:31:27Z

What changes were proposed in this pull request?

rpad and lpad should return NULL when padstring parameter is empty

Why are the changes needed?

Returns str, right-padded or left-padded with pad to a length of length. If str is longer than length, the return value is shortened to length characters. In case of empty pad string, the return value is null.

Different behaviours of rpad and lpad function in case of empty pad string :

Mysql : rpad ('hi', 5, '') => NULL,   lpad ('hi', 5, '') => NULL
Hive : rpad ('hi', 5, '') => NULL,  lpad ('hi', 5, '') => NULL
Oracle : rpad('hi', 5, '') => NULL,   lpad ('hi', 5, '') => NULL
Postgresql : rpad ('hi', 5, '') => hi,  lpad ('hi', 5, '') => hi
presto> select rpad('hi', 5, '');  select lpad('hi', 5, '');
Query 20191225_040937_00004_htqqc failed: Padding string must not be empty

Note : Implemented RPAD and LPAD function as per the definition.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Old unit tests correct as per this jira.

07ARB · 2019-11-12T06:49:05Z

@HyukjinKwon , please verify this PR.

07ARB · 2019-11-13T10:44:55Z

@HyukjinKwon, this we can fix it now ?

…9776

07ARB · 2019-12-24T04:35:28Z

@HyukjinKwon , please verify this PR.

07ARB · 2019-12-24T04:37:24Z

I refer other DBMS and analyse in other project like HIVE they have fixed this issue.
It's good to handle in spark.

maropu · 2019-12-24T04:45:16Z

Can you add end-to-end tests, too, before accepting tests?

07ARB · 2019-12-24T05:41:31Z

ok @maropu

07ARB · 2019-12-24T12:33:35Z

@maropu and @cloud-fan , i have modified exiting end to end test cases , please review again.
kindly inform if i missed anything

sql/core/src/test/resources/sql-tests/results/postgreSQL/strings.sql.out

maropu · 2019-12-25T04:22:11Z

ok to test

maropu · 2019-12-25T04:23:15Z

Also, you need to update the migration guide because this is a behaviour change.

07ARB · 2019-12-25T04:24:25Z

sure i will update the migration guide also.

sql/core/src/test/resources/sql-tests/results/postgreSQL/strings.sql.out

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

maropu · 2019-12-25T04:46:19Z

Can you update the description and the examples for StringRPad, too?
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala#L1244-L1259

maropu · 2019-12-25T04:46:39Z

cc: @HyukjinKwon

SparkQA · 2019-12-25T06:05:52Z

Test build #115758 has finished for PR 26477 at commit 69cd167.

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

07ARB · 2019-12-25T09:45:18Z

@maropu , I have fixed all the review comments, please check.

SparkQA · 2019-12-25T11:33:51Z

Test build #115762 has finished for PR 26477 at commit 628c070.

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

…s empty

…9776

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/strings.sql

SparkQA · 2019-12-26T15:37:32Z

Test build #115813 has finished for PR 26477 at commit def8a0c.

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

srowen · 2019-12-26T16:04:49Z

What about lpad too?

07ARB · 2019-12-26T16:50:09Z

@srowen , for lpad SPARK-29853 this jira is there, after this i will reopen PR for lpad.

srowen · 2019-12-26T17:15:43Z

These are so closely related that they should be one PR and JIRA

07ARB · 2019-12-26T17:16:52Z

ok then i will combine both PR.

…rameter is empty

07ARB · 2019-12-26T18:54:21Z

@srowen and @maropu , i have updated the code for lpad function also, please review.
(I have handled all the above review comments during lpad function implementation).

SparkQA · 2019-12-26T19:52:03Z

Test build #115818 has finished for PR 26477 at commit 62604e4.

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

docs/sql-migration-guide.md

SparkQA · 2019-12-26T23:01:08Z

Test build #115820 has finished for PR 26477 at commit b7d232f.

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

…rameter is empty

SparkQA · 2019-12-27T01:22:31Z

Test build #115821 has finished for PR 26477 at commit e5be2fe.

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

SparkQA · 2019-12-27T02:17:55Z

Test build #115822 has finished for PR 26477 at commit 51247b3.

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

HyukjinKwon · 2019-12-27T03:42:54Z

common/unsafe/src/test/java/org/apache/spark/unsafe/types/UTF8StringSuite.java

-    assertEquals(fromString("数据砖头"), fromString("数据砖头").rpad(5, EMPTY_UTF8));
-    assertEquals(fromString("数据砖"), fromString("数据砖头").rpad(3, EMPTY_UTF8));
-    assertEquals(EMPTY_UTF8, EMPTY_UTF8.rpad(3, EMPTY_UTF8));
+    assertEquals(fromString(null), fromString("数据砖头").rpad(5, EMPTY_UTF8));


So, the previous behaivour was like PostgreSQL before but you propose to match it to Hive, Mysql and Oracle?

If that's the case, I am less sure why we should necessarily change. The lpad and rpad implementations seem different per DBMS implementation. Spark's case at least has one reference and the current behaviour makes sense as well.

It is reasonable to change this because as per description of this function
In case of empty pad string, the return value should be null.

@HyukjinKwon, I feel reasonable because in other DBMS like hive, Mysql they handle this issue after they find this problem.

This function behaviour varies per DBMS implementations and we have a reference as you described, PostgreSQL. Can you guys elaborate why it looks reasonable to you guys? I don't see a strong reason to change the current behaviour.

I think NULL is the better choice, that way we don't return unpadded data.
NULL makes sense with invalid input.
Please refer the JIRA how Hive handled
https://issues.apache.org/jira/browse/HIVE-15792
So as per me we should follow this as per Hive.

cloud-fan · 2019-12-27T07:46:03Z

I agree that NULL makes more sense in this case, but returning the original string is not a bad idea. I don't have a strong opinion on this one. cc @gatorsmile @viirya @dongjoon-hyun

srowen · 2019-12-27T16:28:41Z

Same here, I guess I wouldn't change it if it's not clear whether before or after is better.

viirya · 2019-12-27T18:59:20Z

I have the same option. This looks not a strong reason to change. We can change if it is a standard. Seems there are different implementation across different DBs.

07ARB · 2019-12-28T05:58:51Z

Thank you all, for such a good discussion, If we are not finding strong reason to change old behaviours, then we can close this PR.
One more issue related to Lpad/Rpad, please review #27024

HyukjinKwon · 2019-12-28T09:48:48Z

Closing this as discussed.

maropu · 2019-12-28T09:52:43Z

+1 for the close.

[SPARK-29776][SQL] rpad returning invalid value when parameter is empty

1cb4d33

07ARB mentioned this pull request Nov 12, 2019

[SPARK-29853][SQL]lpad returning empty instead of NULL for empty pad … #26478

Closed

07ARB closed this Nov 12, 2019

07ARB added 2 commits December 22, 2019 22:48

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

c383502

…9776

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

e0208ee

…9776

07ARB reopened this Dec 24, 2019

07ARB force-pushed the SPARK-29776 branch from 97e0ed2 to 69cd167 Compare December 24, 2019 12:30

maropu reviewed Dec 25, 2019

View reviewed changes

sql/core/src/test/resources/sql-tests/results/postgreSQL/strings.sql.out Outdated Show resolved Hide resolved

maropu reviewed Dec 25, 2019

View reviewed changes

sql/core/src/test/resources/sql-tests/results/postgreSQL/strings.sql.out Outdated Show resolved Hide resolved

07ARB changed the title ~~[SPARK-29776][SQL] rpad returning invalid value when parameter is empty~~ [SPARK-29776][SQL] rpad should return NULL when padstring parameter is empty Dec 25, 2019

maropu reviewed Dec 25, 2019

View reviewed changes

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

07ARB force-pushed the SPARK-29776 branch from 69cd167 to 628c070 Compare December 25, 2019 09:42

[SPARK-29776][SQL] rpad should return NULL when padstring parameter i…

af62fa3

…s empty

07ARB force-pushed the SPARK-29776 branch from 628c070 to af62fa3 Compare December 25, 2019 17:36

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

b31b51d

…9776

maropu reviewed Dec 26, 2019

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/postgreSQL/strings.sql Outdated Show resolved Hide resolved

07ARB force-pushed the SPARK-29776 branch from 3549afe to def8a0c Compare December 26, 2019 13:07

07ARB requested a review from maropu December 26, 2019 13:33

[SPARK-29776][SQL] rpad and lpad should return NULL when padstring pa…

62604e4

…rameter is empty

07ARB force-pushed the SPARK-29776 branch from def8a0c to 62604e4 Compare December 26, 2019 18:10

07ARB changed the title ~~[SPARK-29776][SQL] rpad should return NULL when padstring parameter is empty~~ [SPARK-29776][SQL] rpad and lpad should return NULL when padstring parameter is empty Dec 26, 2019

maropu reviewed Dec 26, 2019

View reviewed changes

docs/sql-migration-guide.md Outdated Show resolved Hide resolved

[SPARK-29776][SQL] rpad and lpad should return NULL when padstring pa…

51247b3

…rameter is empty

07ARB force-pushed the SPARK-29776 branch from e5be2fe to 51247b3 Compare December 26, 2019 23:40

07ARB requested a review from maropu December 26, 2019 23:46

HyukjinKwon reviewed Dec 27, 2019

View reviewed changes

07ARB requested a review from HyukjinKwon December 27, 2019 05:49

HyukjinKwon closed this Dec 28, 2019

dongjoon-hyun added the SQL label Feb 5, 2020

[SPARK-29776][SQL] rpad and lpad should return NULL when padstring parameter is empty #26477

[SPARK-29776][SQL] rpad and lpad should return NULL when padstring parameter is empty #26477

Uh oh!

Conversation

07ARB commented Nov 12, 2019 • 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!

07ARB commented Nov 12, 2019

Uh oh!

07ARB commented Nov 13, 2019

Uh oh!

07ARB commented Dec 24, 2019

Uh oh!

07ARB commented Dec 24, 2019

Uh oh!

maropu commented Dec 24, 2019

Uh oh!

07ARB commented Dec 24, 2019

Uh oh!

07ARB commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

maropu commented Dec 25, 2019

Uh oh!

maropu commented Dec 25, 2019

Uh oh!

07ARB commented Dec 25, 2019

Uh oh!

Uh oh!

Uh oh!

maropu commented Dec 25, 2019

Uh oh!

maropu commented Dec 25, 2019

Uh oh!

SparkQA commented Dec 25, 2019

Uh oh!

07ARB commented Dec 25, 2019

Uh oh!

SparkQA commented Dec 25, 2019

Uh oh!

Uh oh!

SparkQA commented Dec 26, 2019

Uh oh!

srowen commented Dec 26, 2019

Uh oh!

07ARB commented Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srowen commented Dec 26, 2019

Uh oh!

07ARB commented Dec 26, 2019

Uh oh!

07ARB commented Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Dec 26, 2019

Uh oh!

Uh oh!

SparkQA commented Dec 26, 2019

Uh oh!

SparkQA commented Dec 27, 2019

Uh oh!

SparkQA commented Dec 27, 2019

Uh oh!

HyukjinKwon Dec 27, 2019

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Dec 27, 2019

Choose a reason for hiding this comment

Uh oh!

AbhishekNew Dec 27, 2019

Choose a reason for hiding this comment

Uh oh!

07ARB Dec 27, 2019

Choose a reason for hiding this comment

07ARB commented Nov 12, 2019 •

edited

Loading

07ARB commented Dec 24, 2019 •

edited

Loading

07ARB commented Dec 26, 2019 •

edited

Loading

07ARB commented Dec 26, 2019 •

edited

Loading