Skip to content

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Apr 19, 2016

What changes were proposed in this pull request?

This issue aims to expose Scala bround function in Python/R API.
bround function is implemented in SPARK-14614 by extending current round function.
We used the following semantics from Hive.

public static double bround(double input, int scale) {
    if (Double.isNaN(input) || Double.isInfinite(input)) {
      return input;
    }
    return BigDecimal.valueOf(input).setScale(scale, RoundingMode.HALF_EVEN).doubleValue();
}

After this PR, pyspark and sparkR also support bround function.

PySpark

>>> from pyspark.sql.functions import bround
>>> sqlContext.createDataFrame([(2.5,)], ['a']).select(bround('a', 0).alias('r')).collect()
[Row(r=2.0)]

SparkR

> df = createDataFrame(sqlContext, data.frame(x = c(2.5, 3.5)))
> head(collect(select(df, bround(df$x, 0))))
  bround(x, 0)
1            2
2            4

How was this patch tested?

Pass the Jenkins tests (including new testcases).

@SparkQA
Copy link

SparkQA commented Apr 19, 2016

Test build #56283 has finished for PR 12509 at commit e63e1fa.

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

Copy link
Member

Choose a reason for hiding this comment

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

is this changing the existing behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, @felixcheung !
I just tried to match the example between round and bround.
round does not change anything.

@SparkQA
Copy link

SparkQA commented Apr 19, 2016

Test build #56286 has finished for PR 12509 at commit d5c0b3d.

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

@SparkQA
Copy link

SparkQA commented Apr 19, 2016

Test build #56291 has finished for PR 12509 at commit fef1bb2.

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

@dongjoon-hyun dongjoon-hyun changed the title [WIP][SPARK-14639] Add bround function in Python/R. [SPARK-14639] Add bround function in Python/R. Apr 19, 2016
@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Apr 20, 2016

Hi, @davies .
Could you review this PR? After your comments, I studied and finally learned how to expose function in Python and R.

And, if you don't mind, may I include some code of minor PR #12329 here too? That PR is so minor and needed to be merged other PR like this. If then, I will just close that PR.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-14639] Add bround function in Python/R. [SPARK-14639][PYTHON][R] Add bround function in Python/R. Apr 20, 2016
#' @export
#' @examples \dontrun{bround(df$c, 0)}
setMethod("bround",
signature(x = "Column", scale = "numeric"),
Copy link
Member

Choose a reason for hiding this comment

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

should scale be default to 0 to be consistent with Python?
to do that, do

           signature(x = "Column"),
           function(x, scale = 0),

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, thank you for sharing know-how. I didn't know that how to do!
I will fix right now.

#' @name bround
#' @family math_funcs
#' @export
#' @examples \dontrun{bround(df$c, 0)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you give a more detailed example of where it gets rounded down and where it gets rounded up ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. Thank you for review. I'll add more example.

@SparkQA
Copy link

SparkQA commented Apr 20, 2016

Test build #56298 has finished for PR 12509 at commit 58a0558.

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

@davies
Copy link
Contributor

davies commented Apr 20, 2016

LGTM

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Apr 20, 2016

Thank you, @davies !
Oh, may I add the minor PR to here? Last time, I remember that you prefer that way.

@dongjoon-hyun
Copy link
Member Author

I mean #12329 . I want to close that one in order to reduce committers' review & merge cost.

@SparkQA
Copy link

SparkQA commented Apr 20, 2016

Test build #56319 has finished for PR 12509 at commit 077633f.

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

@davies
Copy link
Contributor

davies commented Apr 20, 2016

Merging this into master, thanks!

@asfgit asfgit closed this in 14869ae Apr 20, 2016
@dongjoon-hyun dongjoon-hyun deleted the SPARK-14639 branch May 12, 2016 00:59
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.

5 participants