[SPARK-32559][SQL] Fix the trim logic did't handle ASCII control characters correctly #41535

beryllw · 2023-06-09T09:24:05Z

What changes were proposed in this pull request?

The trim logic in Cast expression introduced in #29375 trim ASCII control characters unexpectly.

Before this patch

And hive

Why are the changes needed?

The behavior described above doesn't consistent with the behavior of Hive

Does this PR introduce any user-facing change?

Yes

How was this patch tested?

add ut

beryllw · 2023-06-09T09:30:02Z

@cloud-fan @yaooqinn @gengliangwang @WangGuangxin Could you please help review this?

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

srowen · 2023-06-09T15:25:58Z

I'm not sure this is correct. SQL TRIM from other DBs seems to default to removing whitespace or the space char, not control codes.

beryllw · 2023-06-10T02:49:33Z

I'm not sure this is correct. SQL TRIM from other DBs seems to default to removing whitespace or the space char, not control codes.

I conducted tests on both MySQL and Hive, and found that Hive supports trimming control characters and spaces but does not support trimming characters within the range 0x7F to 0x9F. MySQL, on the other hand, supports trimming specific control characters.

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala

yaooqinn

As Spark is a general engine supporting various kinds of data formats and sources, IMO, it's necessary to trim controls that are very likely to be involved during data exchange.

So, LGTM.

…/DateTimeUtilsSuite.scala Co-authored-by: Kent Yao <yao@apache.org>

…acters correctly ### What changes were proposed in this pull request? The trim logic in Cast expression introduced in #29375 trim ASCII control characters unexpectly. Before this patch ![image](https://github.com/apache/spark/assets/25627922/ca6a2fb1-2143-4264-84d1-70b6bb755ec7) And hive ![image](https://github.com/apache/spark/assets/25627922/017aaa4a-133e-4396-9694-79f03f027bbe) ### Why are the changes needed? The behavior described above doesn't consistent with the behavior of Hive ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? add ut Closes #41535 from Kwafoor/trim_bugfix. Lead-authored-by: wangjunbo <wangjunbo@qiyi.com> Co-authored-by: Junbo wang <1042815068@qq.com> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit 80588e4) Signed-off-by: Kent Yao <yao@apache.org>

yaooqinn · 2023-06-13T03:46:14Z

Thanks, merged to master and 3.4/3.3

…acters correctly ### What changes were proposed in this pull request? The trim logic in Cast expression introduced in apache#29375 trim ASCII control characters unexpectly. Before this patch ![image](https://github.com/apache/spark/assets/25627922/ca6a2fb1-2143-4264-84d1-70b6bb755ec7) And hive ![image](https://github.com/apache/spark/assets/25627922/017aaa4a-133e-4396-9694-79f03f027bbe) ### Why are the changes needed? The behavior described above doesn't consistent with the behavior of Hive ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? add ut Closes apache#41535 from Kwafoor/trim_bugfix. Lead-authored-by: wangjunbo <wangjunbo@qiyi.com> Co-authored-by: Junbo wang <1042815068@qq.com> Signed-off-by: Kent Yao <yao@apache.org>

dongjoon-hyun

Hi, @Kwafoor , @yaooqinn and all.
SPARK-32559 is delivered as Apache Spark 3.0.1 patch.
We need a new JIRA issue because this will land at Apache Spark 3.5.0/3.4.1/3.3.3 newly.

yaooqinn · 2023-06-20T07:07:16Z

@dongjoon-hyun Oops, My bad. For the current status, is it OK to link this pull request to a new jira?

…acters correctly ### What changes were proposed in this pull request? The trim logic in Cast expression introduced in apache#29375 trim ASCII control characters unexpectly. Before this patch ![image](https://github.com/apache/spark/assets/25627922/ca6a2fb1-2143-4264-84d1-70b6bb755ec7) And hive ![image](https://github.com/apache/spark/assets/25627922/017aaa4a-133e-4396-9694-79f03f027bbe) ### Why are the changes needed? The behavior described above doesn't consistent with the behavior of Hive ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? add ut Closes apache#41535 from Kwafoor/trim_bugfix. Lead-authored-by: wangjunbo <wangjunbo@qiyi.com> Co-authored-by: Junbo wang <1042815068@qq.com> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit 80588e4) Signed-off-by: Kent Yao <yao@apache.org>

beryllw · 2023-07-12T09:21:57Z

Hi, @Kwafoor , @yaooqinn and all. SPARK-32559 is delivered as Apache Spark 3.0.1 patch. We need a new JIRA issue because this will land at Apache Spark 3.5.0/3.4.1/3.3.3 newly.

My mistake, should I raise a new issue to link this pr?

yaooqinn · 2023-07-12T09:26:33Z

@Kwafoor Yes

beryllw · 2023-07-12T09:38:10Z

@Kwafoor Yes

I created an issue https://issues.apache.org/jira/browse/SPARK-44383, but how to link this pull request to a new jira？

I saw that spark 3.4.1 was released, with this pr and the wrong issue name SPARK-32559.I’m very sorry about this.

yaooqinn · 2023-07-12T10:23:01Z

I have manually resolved and re-targeted the links.

We probably need to push a new PR to the spark-website repo to fix the release note @Kwafoor

beryllw · 2023-07-13T02:33:20Z

I have manually resolved and re-targeted the links.

We probably need to push a new PR to the spark-website repo to fix the release note @Kwafoor

I created PR-466 to the spark-website repo to fix the release note.

yaooqinn · 2023-07-13T02:35:05Z

thank you @Kwafoor

…acters correctly ### What changes were proposed in this pull request? The trim logic in Cast expression introduced in apache#29375 trim ASCII control characters unexpectly. Before this patch ![image](https://github.com/apache/spark/assets/25627922/ca6a2fb1-2143-4264-84d1-70b6bb755ec7) And hive ![image](https://github.com/apache/spark/assets/25627922/017aaa4a-133e-4396-9694-79f03f027bbe) ### Why are the changes needed? The behavior described above doesn't consistent with the behavior of Hive ### Does this PR introduce _any_ user-facing change? Yes ### How was this patch tested? add ut Closes apache#41535 from Kwafoor/trim_bugfix. Lead-authored-by: wangjunbo <wangjunbo@qiyi.com> Co-authored-by: Junbo wang <1042815068@qq.com> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit 80588e4) Signed-off-by: Kent Yao <yao@apache.org>

github-actions bot added the SQL label Jun 9, 2023

beryllw changed the title ~~Fix the trim logic did't handle ASCII control characters correctly~~ [SPARK-32559]Fix the trim logic did't handle ASCII control characters correctly Jun 9, 2023

yaooqinn reviewed Jun 9, 2023

View reviewed changes

common/unsafe/src/main/java/org/apache/spark/unsafe/types/UTF8String.java Outdated Show resolved Hide resolved

Fix the trim logic did't handle ASCII control characters correctly

184e28a

beryllw force-pushed the trim_bugfix branch from 1b14477 to 184e28a Compare June 9, 2023 11:54

Adjust the code to adhere to Checkstyle guidelines.

bf281e9

beryllw force-pushed the trim_bugfix branch from 41ca2d4 to bf281e9 Compare June 10, 2023 15:52

yaooqinn changed the title ~~[SPARK-32559]Fix the trim logic did't handle ASCII control characters correctly~~ [SPARK-32559][SQL] Fix the trim logic did't handle ASCII control characters correctly Jun 12, 2023

yaooqinn reviewed Jun 12, 2023

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala Outdated Show resolved Hide resolved

yaooqinn approved these changes Jun 12, 2023

View reviewed changes

Update sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util…

4a4ba04

…/DateTimeUtilsSuite.scala Co-authored-by: Kent Yao <yao@apache.org>

github-actions bot added BUILD CONNECT CORE ML PANDAS API ON SPARK PYTHON STRUCTURED STREAMING YARN labels Jun 12, 2023

beryllw force-pushed the trim_bugfix branch from 4125370 to 4a4ba04 Compare June 12, 2023 04:59

github-actions bot removed ML PYTHON BUILD PANDAS API ON SPARK CORE CONNECT YARN labels Jun 12, 2023

github-actions bot removed the STRUCTURED STREAMING label Jun 12, 2023

yaooqinn closed this in 80588e4 Jun 13, 2023

beryllw deleted the trim_bugfix branch June 14, 2023 02:06

dongjoon-hyun reviewed Jun 19, 2023

View reviewed changes

beryllw mentioned this pull request Jul 13, 2023

Fix the link of PR-41535 in Spark 3.4.1 release note apache/spark-website#466

Merged

This was referenced Oct 31, 2024

[VL][1.2][Result mismatch] Cast string to integral type does not ignore ISO control characters apache/incubator-gluten#7749

Closed

[GLUTEN-7749][VL] Trim ISOControl characters in string for casting to integral type apache/incubator-gluten#7806

Merged

[SPARK-32559][SQL] Fix the trim logic did't handle ASCII control characters correctly #41535

[SPARK-32559][SQL] Fix the trim logic did't handle ASCII control characters correctly #41535

Uh oh!

Conversation

beryllw commented Jun 9, 2023 • 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!

beryllw commented Jun 9, 2023

Uh oh!

Uh oh!

srowen commented Jun 9, 2023

Uh oh!

beryllw commented Jun 10, 2023

Uh oh!

Uh oh!

yaooqinn left a comment

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Jun 13, 2023

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beryllw commented Jul 12, 2023

Uh oh!

yaooqinn commented Jul 12, 2023

Uh oh!

beryllw commented Jul 12, 2023

Uh oh!

yaooqinn commented Jul 12, 2023

Uh oh!

beryllw commented Jul 13, 2023

Uh oh!

yaooqinn commented Jul 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

beryllw commented Jun 9, 2023 •

edited

Loading

yaooqinn commented Jun 20, 2023 •

edited

Loading