Skip to content

[SPARK-21016][core]Improve code fault tolerance for converting string to number#18238

Closed
10110346 wants to merge 1 commit into
apache:masterfrom
10110346:lx-wip-0608
Closed

[SPARK-21016][core]Improve code fault tolerance for converting string to number#18238
10110346 wants to merge 1 commit into
apache:masterfrom
10110346:lx-wip-0608

Conversation

@10110346

@10110346 10110346 commented Jun 8, 2017

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

When converting string to number(int, long or double), if the string has a space before or after,will lead to unnecessary mistakes.

How was this patch tested?

unit test

@wzhfy

wzhfy commented Jun 10, 2017

Copy link
Copy Markdown
Contributor

If a string has a leading or trailing space, it's expected it cannot be converted to a number.
Java's string type also follows this convention.

@10110346

10110346 commented Jun 10, 2017

Copy link
Copy Markdown
Contributor Author

But it can be converted to double.
If a string has a leading or trailing space, we are not easy to detect, especially the user configuration,this Pr is to solve this problem.

@kiszk

kiszk commented Jun 10, 2017

Copy link
Copy Markdown
Member

It would be good to do the same thing for s.toBoolean in toBoolean method.

@HyukjinKwon

Copy link
Copy Markdown
Member

I think it will be more persuasive if there are some cases where users mistakenly write wrong values for both numbers and booleans. Personally, I haven't got any errors about this before (more than two years in production / development).

@10110346

Copy link
Copy Markdown
Contributor Author

@HyukjinKwon I agree with you ,but i think if add trim', it would be better

@gatorsmile

Copy link
Copy Markdown
Member

Although it is not a common user error, it does not hurt to add an extra trim.

@gatorsmile

Copy link
Copy Markdown
Member

ok to test

@SparkQA

SparkQA commented Jun 12, 2017

Copy link
Copy Markdown

Test build #77917 has started for PR 18238 at commit 04307c6.

@SparkQA

SparkQA commented Jun 12, 2017

Copy link
Copy Markdown

Test build #77921 has finished for PR 18238 at commit 9317956.

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

@srowen

srowen commented Jun 12, 2017

Copy link
Copy Markdown
Member

I'm neutral. Accepting bad input isn't usually a great idea, though in this case, I can't see much harm. It's not ambiguous.

@SparkQA

SparkQA commented Jun 12, 2017

Copy link
Copy Markdown

Test build #77930 has finished for PR 18238 at commit c09eaa1.

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

@gatorsmile

Copy link
Copy Markdown
Member

retest this please

@SparkQA

SparkQA commented Jun 12, 2017

Copy link
Copy Markdown

Test build #77944 has finished for PR 18238 at commit c09eaa1.

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

@SparkQA

SparkQA commented Jun 13, 2017

Copy link
Copy Markdown

Test build #77966 has started for PR 18238 at commit c09eaa1.

@10110346

Copy link
Copy Markdown
Contributor Author

Jenkins, retest this please

@SparkQA

SparkQA commented Jun 13, 2017

Copy link
Copy Markdown

Test build #77982 has finished for PR 18238 at commit c09eaa1.

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

@gatorsmile

Copy link
Copy Markdown
Member

Thanks! Merging to master.

@asfgit asfgit closed this in 7ba8bf2 Jun 13, 2017
dataknocker pushed a commit to dataknocker/spark that referenced this pull request Jun 16, 2017
…g to number

## What changes were proposed in this pull request?
When converting `string` to `number`(int, long or double),  if the string has a space before or after,will lead to unnecessary mistakes.

## How was this patch tested?
unit test

Author: liuxian <liu.xian3@zte.com.cn>

Closes apache#18238 from 10110346/lx-wip-0608.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants