Skip to content

Conversation

@actuaryzhang
Copy link
Contributor

What changes were proposed in this pull request?

Add SQL trunc function

How was this patch tested?

standard test

@actuaryzhang
Copy link
Contributor Author

actuaryzhang commented Jun 13, 2017

@felixcheung @zero323
Reopening this in a new PR.

@SparkQA
Copy link

SparkQA commented Jun 13, 2017

Test build #77997 has started for PR 18291 at commit ef70cee.

@actuaryzhang
Copy link
Contributor Author

jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Jun 14, 2017

Test build #78020 has finished for PR 18291 at commit ef70cee.

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

Copy link
Member

@felixcheung felixcheung left a comment

Choose a reason for hiding this comment

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

minor comment, LGTM otherwise

"to_utc_timestamp",
"translate",
"trim",
"trunc",
Copy link
Member

Choose a reason for hiding this comment

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

it doesn't mask base::trunc?

Copy link
Member

Choose a reason for hiding this comment

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

and looks like it has a generic already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is part of the internally S4 methods and there is already generics.
This is similar to math functions like abs.
https://stat.ethz.ch/R-manual/R-devel/library/base/html/groupGeneric.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yes, it doesn't mask base. You can still do trunc(10.5).

setMethod("trunc",
signature(x = "Column"),
function(x, format) {
jc <- callJStatic("org.apache.spark.sql.functions", "trunc", x@jc, format)
Copy link
Member

Choose a reason for hiding this comment

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

I'd as.character(format) here

@actuaryzhang
Copy link
Contributor Author

Added your suggested change. Thanks!

@SparkQA
Copy link

SparkQA commented Jun 15, 2017

Test build #78115 has finished for PR 18291 at commit 797c3d0.

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

@actuaryzhang
Copy link
Contributor Author

@felixcheung Anything else needed for this PR?

@felixcheung
Copy link
Member

merged to master.

@asfgit asfgit closed this in 110ce1f Jun 19, 2017
@actuaryzhang actuaryzhang deleted the sparkRTrunc2 branch June 19, 2017 01:14
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.

3 participants