Skip to content

[SPARK-17299] TRIM/LTRIM/RTRIM should not strips characters other than spaces #14924

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
techaddict wants to merge 4 commits into from
Closed

[SPARK-17299] TRIM/LTRIM/RTRIM should not strips characters other than spaces #14924

techaddict wants to merge 4 commits into from

Conversation

@techaddict
Copy link
Contributor

@techaddict techaddict commented Sep 1, 2016

What changes were proposed in this pull request?

TRIM/LTRIM/RTRIM should not strips characters other than spaces, we were trimming all chars small than ASCII 0x20(space)

How was this patch tested?

fixed existing tests.

@SparkQA
Copy link

SparkQA commented Sep 1, 2016

Test build #64798 has finished for PR 14924 at commit 58c2a5d.

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

@rxin
Copy link
Contributor

rxin commented Sep 2, 2016

Can you add a unit test for this?

@techaddict
Copy link
Contributor Author

techaddict commented Sep 2, 2016

@rxin Done 👍

@SparkQA
Copy link

SparkQA commented Sep 2, 2016

Test build #64827 has finished for PR 14924 at commit 554f46c.

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

@srowen
Copy link
Member

srowen commented Sep 2, 2016

The implementation in stringExpressions.scala generates code that calls trim(), so just double-checking, that's calling UTF8String not String right? Then yeah LGTM

@techaddict
Copy link
Contributor Author

techaddict commented Sep 2, 2016 •
edited
Loading

@srowen yes in stringExpressions the trim is UTF8String.trim().

@SparkQA
Copy link

SparkQA commented Sep 2, 2016

Test build #64856 has finished for PR 14924 at commit 7583070.

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

@srowen
Copy link
Member

srowen commented Sep 3, 2016

LGTM. Seems like this should go into 2.0.x as well since it's really a bug fix.

@srowen
Copy link
Member

srowen commented Sep 6, 2016

Merged to master/2.0

1 similar comment
@srowen
Copy link
Member

srowen commented Sep 6, 2016

Merged to master/2.0

@asfgit asfgit closed this in 7775d9f Sep 6, 2016
asfgit pushed a commit that referenced this pull request Sep 6, 2016
@techaddict @srowen
[SPARK-17299] TRIM/LTRIM/RTRIM should not strips characters other tha…
0ae9786 
…n spaces

## What changes were proposed in this pull request?
TRIM/LTRIM/RTRIM should not strips characters other than spaces, we were trimming all chars small than ASCII 0x20(space)

## How was this patch tested?
fixed existing tests.

Author: Sandeep Singh <sandeep@techaddict.me>

Closes #14924 from techaddict/SPARK-17299.

(cherry picked from commit 7775d9f)
Signed-off-by: Sean Owen <sowen@cloudera.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@techaddict @SparkQA @rxin @srowen