[SPARK-29986][SQL] casting string to date/timestamp/interval should trim all whitespaces #26626

yaooqinn · 2019-11-21T10:58:36Z

What changes were proposed in this pull request?

A java like string trim method trims all whitespaces that less or equal than 0x20. currently, our UTF8String handle the space =0x20 ONLY. This is not suitable for many cases in Spark, like trim for interval strings, date, timestamps, PostgreSQL like cast string to boolean.

Why are the changes needed?

improve the white spaces handling in UTF8String, also with some bugs fixed

Does this PR introduce any user-facing change?

yes,
string with control character at either end can be convert to date/timestamp and interval now

How was this patch tested?

add ut

yaooqinn · 2019-11-21T10:59:19Z

cc @cloud-fan @maropu @wangyum @HyukjinKwon thanks in advance.

cloud-fan · 2019-11-21T11:12:07Z

The thing we care is the SQL behavior, not java behavior. Can you check other databases and see if the trim function trims all chars whose ascii code <= 0x20?

yaooqinn · 2019-11-21T11:25:09Z

The thing we care is the SQL behavior, not java behavior. Can you check other databases and see if the trim function trims all chars whose ascii code <= 0x20?

Not changing string functions, just for other types listed in pr desc.
Besides, the former interval parser should be able to handle these control chars

postgres=# select date E'2019-01-01\t';
    date
------------
 2019-01-01
(1 row)

postgres=# select date E'2019-01-01\t';
    date
------------
 2019-01-01
(1 row)

postgres=# select cast(E'1\t' as boolean);
 bool
------
 t
(1 row)

postgres=# select timestamp E'2019-01-01\t';
      timestamp
---------------------
 2019-01-01 00:00:00
(1 row)

hive

select date('2019-10-10\t');
"_c0"
"2019-10-10"

cloud-fan · 2019-11-21T11:45:02Z

can you try select trim('2019-10-10\t') in other databases?

yaooqinn · 2019-11-21T11:58:22Z

can you try select trim('2019-10-10\t') in other databases?

we are not changing string trim here, but here is the test result.

postgres=# select length(trim(E'2019-10-10\t'));
 length
--------
     11
(1 row)

presto> select length(trim('2019-10-10' || CHR(09)));
 _col0
-------
    10
(1 row)

hive
select length(trim('2019-10-10\t')) 
"_c0"
"11"

SparkQA · 2019-11-21T12:49:39Z

Test build #114234 has finished for PR 26626 at commit 0bd8239.

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

srowen · 2019-11-21T12:52:40Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

  }

+  /**
+   * Trims whitespaces (<= ASCII 32) from both ends of this string.


Interesting, so SQL trim just removes space, and java.lang.String.trim() removes everything <= 32. Maybe we could refer to that in this doc, that this is the purpose of this additional trim method.

yea, let's mention it's the same as java.lang.String.trim

...c/main/scala/org/apache/spark/sql/catalyst/expressions/postgreSQL/PostgreCastToBoolean.scala

srowen · 2019-11-21T12:56:17Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

+  /**
+   * Trims whitespaces (<= ASCII 32) from both ends of this string.
+   *
+   * @return this string with no spaces at the start or end


Nit: not just space

yaooqinn · 2019-11-21T14:46:33Z

scala> " 234\t ".toDouble
res0: Double = 234.0

scala> java.lang.Double.valueOf(" 234\t ")
res1: Double = 234.0

the way spark deal with casting string to approximate numeric - float and double also trim control characters silently. @cloud-fan
@srowen

I take a look at SQL standard about string trim

If trim character is not specified, then ' ' is implicit.

b) If is specified, then let SC be the value of trim character; otherwise, let SC be
space.

yaooqinn · 2019-11-21T14:50:00Z

I also look up casting string to other types, the standard only says

If SD is character string, then SV is replaced by TRIM ( BOTH ' ' FROM VE )

It seem that most modern dbs are varying this rule.

SparkQA · 2019-11-21T15:10:52Z

Test build #114243 has finished for PR 26626 at commit 41feca0.

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

cloud-fan · 2019-11-21T15:21:47Z

From SQL standard

let SRC be <trim source>. TRIM ( SRC ) is equivalent to TRIM ( BOTH ' ' FROM SRC ).

cast specification
If SD is character string, then SV is replaced by SV with any leading or trailing <space>s removed.

some related information

L( <left bracket> <colon> SPACE <colon> <right bracket> )
is the set of all character strings of length 1 (one) that are the <space> character.
r) L( <left bracket> <colon> WHITESPACE <colon> <right bracket> )
is the set of all character strings of length 1 (one) that are white space characters.
...
white space
consecutive sequences of one or more characters that have no glyphs

So space means ' ', and white space means all chars whose ascii code <= 32. trim and cast should only remove spaces.

However, seems most of the DBs don't follow the cast part, and we rely on Double.valueOf so hard to change this behavior. I think it's OK to trim white spaces in cast.

SparkQA · 2019-11-21T18:16:59Z

Test build #114247 has finished for PR 26626 at commit 72a2065.

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

cloud-fan · 2019-11-22T03:04:23Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

+      // Nothing trimmed
+      return this;
+    }
+    return copyUTF8String(s, e);


looking at the caller side, I think it's safe to not copy the data. We can add a caveat in the javadoc that: this method doesn't copy the data and the caller side should do copy themselves if they want to hold it for a while.

do we not copy when it is an EMPTY_UTF8 either? A bit odd if we do it differently.

ah that's a good point. OK let's leave it.

cloud-fan · 2019-11-22T05:32:48Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

+   * Trims whitespaces (<= ASCII 32) from both ends of this string.
+   *
+   * Note that, this method is the same as java's {@link String#trim}, and different from
+   * {@link UTF8String#trim()} which only remove only spaces(= ASCII 32) from both ends.


nit: remove one "only"

cloud-fan · 2019-11-22T05:33:16Z

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java

+  public UTF8String trimAll() {
+    int s = 0;
+    // skip all of the whitespaces (<=0x20) in the left side
+    while (s < this.numBytes && getByte(s) <= 0x20) s++;


shall we use ' ' instead of 0x20?

maybe also replace them in method trim? I don't know if whether ok or not I do so in this pr

SparkQA · 2019-11-22T06:43:57Z

Test build #114269 has finished for PR 26626 at commit 79abc93.

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

SparkQA · 2019-11-22T08:05:02Z

Test build #114276 has finished for PR 26626 at commit 773eb4b.

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

yaooqinn · 2019-11-22T08:28:11Z

retest this please

SparkQA · 2019-11-22T10:28:36Z

Test build #114287 has finished for PR 26626 at commit 773eb4b.

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

yaooqinn · 2019-11-22T10:44:15Z

retest this please

SparkQA · 2019-11-22T13:30:44Z

Test build #114292 has finished for PR 26626 at commit 773eb4b.

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

cloud-fan · 2019-11-22T13:51:24Z

LGTM, can we add a migration guide?

yaooqinn · 2019-11-22T14:09:24Z

migration guide added

SparkQA · 2019-11-22T16:31:50Z

Test build #114303 has finished for PR 26626 at commit 3cd5433.

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

cloud-fan · 2019-11-25T06:37:19Z

thanks, merging to master!

yaooqinn · 2019-11-25T06:38:59Z

Thanks for merging and the more suitable PR title :)

[SPARK-29986][SQL] Introduce java like string trim to UTF8String

0bd8239

srowen reviewed Nov 21, 2019

View reviewed changes

nit

41feca0

yaooqinn mentioned this pull request Nov 21, 2019

[SPARK-28023][SQL] Add trim logic in UTF8String's toInt/toLong to make it consistent with other string-numeric casting #26622

Closed

fix ut

72a2065

dongjoon-hyun added the SQL label Nov 21, 2019

cloud-fan reviewed Nov 22, 2019

View reviewed changes

doc

79abc93

cloud-fan reviewed Nov 22, 2019

View reviewed changes

nit

773eb4b

Merge branch 'master' into SPARK-29986

9888e20

m guide

3cd5433

cloud-fan changed the title ~~[SPARK-29986][SQL] Introduce java like string trim to UTF8String~~ [SPARK-29986][SQL] casting string to date/timestamp/interval should trim all whitespaces Nov 25, 2019

cloud-fan closed this in de21f28 Nov 25, 2019

yaooqinn deleted the SPARK-29986 branch November 25, 2019 06:39

[SPARK-29986][SQL] casting string to date/timestamp/interval should trim all whitespaces #26626

[SPARK-29986][SQL] casting string to date/timestamp/interval should trim all whitespaces #26626

Uh oh!

Conversation

yaooqinn commented Nov 21, 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?

Uh oh!

yaooqinn commented Nov 21, 2019

Uh oh!

cloud-fan commented Nov 21, 2019

Uh oh!

yaooqinn commented Nov 21, 2019

hive

Uh oh!

cloud-fan commented Nov 21, 2019

Uh oh!

yaooqinn commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Nov 21, 2019

Uh oh!

yaooqinn commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

cloud-fan commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

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!

SparkQA commented Nov 22, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

yaooqinn commented Nov 22, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

yaooqinn commented Nov 22, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

cloud-fan commented Nov 22, 2019

Uh oh!

yaooqinn commented Nov 22, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

cloud-fan commented Nov 25, 2019

Uh oh!

yaooqinn commented Nov 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development