Skip to content

Conversation

@lgieron
Copy link

@lgieron lgieron commented Feb 26, 2016

What changes were proposed in this pull request?

Change in class FormatNumber to make it work irrespective of locale.

How was this patch tested?

Unit tests.

@srowen
Copy link
Member

srowen commented Feb 26, 2016

That looks good. Are there other instances where a DecimalFormat or NumberFormat doesn't specify a locale? we should fix anything of the same form.

import org.apache.spark.sql.types._
import org.apache.spark.unsafe.types.{ByteArray, UTF8String}
import java.text.DecimalFormatSymbols

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this import should be grouped with the other java.text import.

@zsxwing
Copy link
Member

zsxwing commented Feb 26, 2016

I'm wondering if some people may want to use other Locale, such as 4.000. Why not just fix tests?

@lgieron
Copy link
Author

lgieron commented Feb 26, 2016

@zsxwing This class is the backend for the format_number SQL function, which is supposed to be locale-independent.

@lgieron
Copy link
Author

lgieron commented Feb 26, 2016

@srowen I went through all other uses of DecimalFormat and NumberFormat across all subprojects - they all look correct.

@zsxwing
Copy link
Member

zsxwing commented Feb 26, 2016

@lgieron I see.Thanks for clarifying.

@lgieron
Copy link
Author

lgieron commented Feb 26, 2016

@srowen I've cleaned up the code as per your comments.

package org.apache.spark.sql.catalyst.expressions

import java.text.DecimalFormat
import java.text.DecimalFormatSymbols
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: these should be grouped as java.text.{DecimalFormat, DecimalFormatSymbols}

@srowen
Copy link
Member

srowen commented Feb 27, 2016

Jenkins test this please

@SparkQA
Copy link

SparkQA commented Feb 27, 2016

Test build #52122 has finished for PR 11396 at commit 40e1ec8.

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

@lgieron
Copy link
Author

lgieron commented Feb 27, 2016

@srowen The build failed due to unrelated test failing (https://issues.apache.org/jira/browse/SPARK-13530). The fix was committed to master an hour ago and now the test passes.

@zsxwing
Copy link
Member

zsxwing commented Feb 28, 2016

ok to test

@SparkQA
Copy link

SparkQA commented Feb 28, 2016

Test build #52144 has finished for PR 11396 at commit aab31f9.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@lgieron
Copy link
Author

lgieron commented Feb 28, 2016

@zsxwing Can you please make Jenkins rerun the tests?

@SparkQA
Copy link

SparkQA commented Feb 28, 2016

Test build #52145 has finished for PR 11396 at commit fef157f.

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

@SparkQA
Copy link

SparkQA commented Feb 28, 2016

Test build #52146 has finished for PR 11396 at commit b3b7b15.

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

@srowen
Copy link
Member

srowen commented Feb 29, 2016

Looks good. CC @chenghao-intel to make sure we're not missing anything; I wasn't sure why there was a local DecimalFormat created in the first place. Is the non-thread-safety of the NumberFormat an issue here? I don't think so given my guess about how this is called, but thought I'd ask

@srowen
Copy link
Member

srowen commented Mar 2, 2016

Merged to master

@asfgit asfgit closed this in d8afd45 Mar 2, 2016
roygao94 pushed a commit to roygao94/spark that referenced this pull request Mar 22, 2016
## What changes were proposed in this pull request?

Change in class FormatNumber to make it work irrespective of locale.

## How was this patch tested?

Unit tests.

Author: lgieron <[email protected]>

Closes apache#11396 from lgieron/SPARK-13515_Fix_Format_Number.
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.

4 participants