Skip to content
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.

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

[SPARK-24408][SQL][DOC] Move abs function to math_funcs group #21448

Closed

Conversation

jaceklaskowski
Copy link
Contributor

What changes were proposed in this pull request?

A few math functions (abs , bitwiseNOT, isnan, nanvl) are not in math_funcs group. They should really be.

How was this patch tested?

Awaiting Jenkins

@SparkQA
Copy link

SparkQA commented May 29, 2018

Test build #91247 has finished for PR 21448 at commit 25fe97f.

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

@SparkQA
Copy link

SparkQA commented May 29, 2018

Test build #91250 has finished for PR 21448 at commit 487a467.

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

@jaceklaskowski
Copy link
Contributor Author

It is such a small change that I don't think it's going to take long to get merged. Reaching out to friendly folks to reach a consensus on it :) /cc @srowen @holdenk

@srowen
Copy link
Member

srowen commented May 30, 2018

abs does seem to belong with math functions.

isnan and nanvl are sort of math related but to me feel more like isnull. It's more about testing representations of a value, not math on them (NaN isn't a mathematical notion, note). I'd leave it. Those methods in the JDK aren't in java.lang.Math, FWIW.

bitwiseNOT sure seems like an odd man out. The other bitwise functions are defined in Column.scala I don't see why, other than that 'not' is a unary operator. This came from #5867 (diff) but looks like that was a comment about functions.py. Although its location is odd, I guess I'd leave it unless @Shiti or @rxin say it's OK to move.

@holdenk
Copy link
Contributor

holdenk commented May 30, 2018

If it’s ok I’ll take a quick look at this on Friday as discussed with Jacek.

@rxin
Copy link
Contributor

rxin commented May 30, 2018

I'd only move abs and nothing else.

@holdenk
Copy link
Contributor

holdenk commented Jun 1, 2018

So, I personally don't have strong feelings about which groups these functions should be in, but just a reminder we should unify with PySpark with whatever our decision is.

In PySpark bitwiseNot is under math, abs is currently grouped with some string operators. Less important since its just code organization there but while were thinking about it anyways.

@holdenk
Copy link
Contributor

holdenk commented Jun 22, 2018

@jaceklaskowski how would you feel about just moving abs as suggested by @rxin?

@holdenk
Copy link
Contributor

holdenk commented Jun 28, 2018

Cool, let me know when you get a chance to update the PR @jaceklaskowski :)

@jaceklaskowski jaceklaskowski changed the title [SPARK-24408][SQL][DOC] Move abs, bitwiseNOT, isnan, nanvl functions to math_funcs group [SPARK-24408][SQL][DOC] Move abs function to math_funcs group Jun 28, 2018
Copy link
Contributor

@holdenk holdenk left a comment

Choose a reason for hiding this comment

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

LGTM

@holdenk
Copy link
Contributor

holdenk commented Jun 28, 2018

Merged to master

@asfgit asfgit closed this in e1d3f80 Jun 28, 2018
@SparkQA
Copy link

SparkQA commented Jun 28, 2018

Test build #92437 has finished for PR 21448 at commit d11dbb9.

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

@SparkQA
Copy link

SparkQA commented Jun 29, 2018

Test build #92438 has finished for PR 21448 at commit 2bbc750.

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

@jaceklaskowski jaceklaskowski deleted the SPARK-24408-math-funcs-doc branch July 1, 2018 20:42
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