Skip to content

[SPARK-8217] [SQL] math function log2 #6718

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[SPARK-8217] [SQL] math function log2 #6718

wants to merge 1 commit into from

Conversation

@adrian-wang
Copy link
Contributor

@adrian-wang adrian-wang commented Jun 9, 2015

No description provided.

@SparkQA
Copy link

SparkQA commented Jun 9, 2015

Test build #34494 has finished for PR 6718 at commit 49454da.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Log2(child: Expression)

@SparkQA
Copy link

SparkQA commented Jun 9, 2015

Test build #34497 has finished for PR 6718 at commit 688e76a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Log2(child: Expression)
    • logInfo(s"Using user defined output committer class $
    • logInfo(s"Using output committer class $

@SparkQA
Copy link

SparkQA commented Jun 9, 2015

Test build #34502 has finished for PR 6718 at commit 9408d93.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class SimpleFunctionRegistry extends FunctionRegistry
    • case class Log2(child: Expression)
    • case class Rand(seed: Long) extends RDG(seed)
    • case class Randn(seed: Long) extends RDG(seed)
    • class StringKeyHashMap[T](normalizer: (String) => String)

rxin
rxin reviewed Jun 9, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated
Copy link
Contributor

@rxin rxin Jun 9, 2015

Choose a reason for hiding this comment

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

why upper case Base?

Copy link
Contributor Author

@adrian-wang adrian-wang Jun 10, 2015

Choose a reason for hiding this comment

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

because we use upper case Base for log10 also, I just try to keep consistence.
should I modify it to lower case also?

Copy link
Contributor

@rxin rxin Jun 10, 2015

Choose a reason for hiding this comment

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

yes change them all. not sure why they were "Base" before.

@rxin
Copy link
Contributor

rxin commented Jun 9, 2015

Can you also add to functions.py?

rxin
rxin reviewed Jun 9, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated
Copy link
Contributor

@rxin rxin Jun 9, 2015

Choose a reason for hiding this comment

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

can we name the argument a?

Copy link
Contributor Author

@adrian-wang adrian-wang Jun 10, 2015

Choose a reason for hiding this comment

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

This is also following log/log10/log1p. Change them all?

Copy link
Contributor

@rxin rxin Jun 10, 2015

Choose a reason for hiding this comment

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

yes change them all. i'm not sure why e was used.

@rxin
Copy link
Contributor

rxin commented Jun 10, 2015

Please also add unit tests to DataFrameFunctionSuite (like what @chenghao-intel did in his length pr)

chenghao-intel
chenghao-intel reviewed Jun 10, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated
Copy link
Contributor

@chenghao-intel chenghao-intel Jun 10, 2015

Choose a reason for hiding this comment

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

=> the given column

@chenghao-intel
Copy link
Contributor

chenghao-intel commented Jun 10, 2015

And also add sql query for unittest, just be sure the UDF registration works.

rxin
rxin reviewed Jun 10, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated
Copy link
Contributor

@rxin rxin Jun 10, 2015

Choose a reason for hiding this comment

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

actually after thinking about it more ... this breaks scala source compatibility. Do you mind switching back to "e" and just use "e" for everything? It might've meant "expression".

For the new ones, I'd use "expr".

Copy link
Contributor Author

@adrian-wang adrian-wang Jun 10, 2015

Choose a reason for hiding this comment

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

ok...

@rxin
Copy link
Contributor

rxin commented Jun 10, 2015

LGTM other than one test.

@SparkQA
Copy link

SparkQA commented Jun 10, 2015

Test build #34563 has finished for PR 6718 at commit 29ec6c9.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Log2(child: Expression)

@SparkQA
Copy link

SparkQA commented Jun 10, 2015

Test build #34565 has finished for PR 6718 at commit bce0191.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Log2(child: Expression)

@SparkQA
Copy link

SparkQA commented Jun 10, 2015

Test build #34568 has finished for PR 6718 at commit 801c91d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Log2(child: Expression)

@SparkQA
Copy link

SparkQA commented Jun 10, 2015

Test build #34578 has finished for PR 6718 at commit 00e6df3.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Log2(child: Expression)

rxin
rxin reviewed Jun 10, 2015
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/DataFrameFunctionsSuite.scala Outdated
Copy link
Contributor

@rxin rxin Jun 10, 2015

Choose a reason for hiding this comment

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

log2 -- not log

@rxin
Copy link
Contributor

rxin commented Jun 11, 2015

LGTM.

@SparkQA
Copy link

SparkQA commented Jun 11, 2015

Test build #34651 has finished for PR 6718 at commit 3909f48.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Log2(child: Expression)

@rxin
Copy link
Contributor

rxin commented Jun 11, 2015

Thanks -- I manually resolved the conflict and merged this.

@asfgit asfgit closed this in 2758ff0 Jun 11, 2015
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
@adrian-wang @nemccarthy
[SPARK-8217] [SQL] math function log2
783b4e4 
Author: Daoyuan Wang <[email protected]>

This patch had conflicts when merged, resolved by
Committer: Reynold Xin <[email protected]>

Closes apache#6718 from adrian-wang/udflog2 and squashes the following commits:

3909f48 [Daoyuan Wang] math function: log2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@adrian-wang @SparkQA @rxin @chenghao-intel @davies