[SPARK-8363][SQL] Move sqrt to math and extend UnaryMathExpression #6823

viirya · 2015-06-15T06:54:56Z

JIRA: https://issues.apache.org/jira/browse/SPARK-8363

SparkQA · 2015-06-15T07:17:50Z

Test build #34917 has finished for PR 6823 at commit ef4a21a.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Sqrt(child: Expression) extends UnaryMathExpression(math.sqrt, "SQRT")

SparkQA · 2015-06-15T07:49:30Z

Test build #34919 has finished for PR 6823 at commit 297cc90.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Sqrt(child: Expression) extends UnaryMathExpression(math.sqrt, "SQRT")

…oercion.

cloud-fan · 2015-06-15T08:12:54Z

...yst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ArithmeticExpressionSuite.scala

I think we need to move this test case from arithmetic cases to math test cases.

math test cases already have similar tests. If we don't need it here, we can remove it from arithmetic cases.

cloud-fan · 2015-06-15T08:39:54Z

Hi @viirya, could you also remove the type check test for Sqrt here? And remove the type cast rule for Sqrt here

SparkQA · 2015-06-15T11:14:52Z

Test build #34921 has finished for PR 6823 at commit d38492f.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Sqrt(child: Expression) extends UnaryMathExpression(math.sqrt, "SQRT")

…est. Remove unnecessary Sqrt type rule.

SparkQA · 2015-06-15T15:21:37Z

Test build #34939 has finished for PR 6823 at commit bc2ed77.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Sqrt(child: Expression) extends UnaryMathExpression(math.sqrt, "SQRT")

rxin · 2015-06-18T06:44:03Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/MathFunctionsSuite.scala

isn't 2^24 super large? why are we doing this in a unit test?

They are moved from the test of sqrt in ArithmeticExpressionSuite. Is it better to directly remove them?

I think we can just pick a few numbers out of that. There isn't much benefit to test a lot of numbers here.

Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala

viirya · 2015-06-18T07:08:40Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

Ah, I think this should be 1.5.0 too.

rxin · 2015-06-18T07:12:10Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/MathFunctionsSuite.scala

sorry what I meant was writing out the sqrt value explicitly ... rather than relying on some sequence

ok. sorry I misunderstand that.

rxin · 2015-06-18T07:35:40Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/MathFunctionsSuite.scala

actually - question: why having this when you already have the testUnary call up there?

maybe this is doing something I am not understanding yet

They are old tests in ArithmeticExpressionSuite. I think we can remove them.

SparkQA · 2015-06-18T08:16:35Z

Test build #35111 has finished for PR 6823 at commit 8977e11.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Sqrt(child: Expression) extends UnaryMathExpression(math.sqrt, "SQRT")

SparkQA · 2015-06-18T09:03:03Z

Test build #35102 has finished for PR 6823 at commit 8dff6d1.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Sqrt(child: Expression) extends UnaryMathExpression(math.sqrt, "SQRT")

SparkQA · 2015-06-18T09:04:09Z

Test build #35103 has finished for PR 6823 at commit 699f48b.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class ElementwiseProduct(VectorTransformer):
- case class Sqrt(child: Expression) extends UnaryMathExpression(math.sqrt, "SQRT")
- // Add jar to isolated hive (metadataHive) class loader.

SparkQA · 2015-06-18T09:19:17Z

Test build #35106 has finished for PR 6823 at commit d23e79e.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class Sqrt(child: Expression) extends UnaryMathExpression(math.sqrt, "SQRT")

cloud-fan · 2015-06-18T10:07:01Z

LGTM

rxin · 2015-06-18T20:00:22Z

Thanks. Merging this.

rxin · 2015-06-18T20:01:14Z

Can you add the Python version for this?

@SInCE

JIRA: https://issues.apache.org/jira/browse/SPARK-8363 Author: Liang-Chi Hsieh <[email protected]> Closes apache#6823 from viirya/move_sqrt and squashes the following commits: 8977e11 [Liang-Chi Hsieh] Remove unnecessary old tests. d23e79e [Liang-Chi Hsieh] Explicitly indicate sqrt value sequence. 699f48b [Liang-Chi Hsieh] Use correct @SInCE tag. 8dff6d1 [Liang-Chi Hsieh] Merge remote-tracking branch 'upstream/master' into move_sqrt bc2ed77 [Liang-Chi Hsieh] Remove/move arithmetic expression test and expression type checking test. Remove unnecessary Sqrt type rule. d38492f [Liang-Chi Hsieh] Now sqrt accepts boolean because type casting is handled by HiveTypeCoercion. 297cc90 [Liang-Chi Hsieh] Sqrt only accepts double input. ef4a21a [Liang-Chi Hsieh] Move sqrt to math.

Move sqrt to math.

ef4a21a

Sqrt only accepts double input.

297cc90

Now sqrt accepts boolean because type casting is handled by HiveTypeC…

d38492f

…oercion.

cloud-fan reviewed Jun 15, 2015
View reviewed changes

Remove/move arithmetic expression test and expression type checking t…

bc2ed77

…est. Remove unnecessary Sqrt type rule.

rxin reviewed Jun 18, 2015
View reviewed changes

Merge remote-tracking branch 'upstream/master' into move_sqrt

8dff6d1

Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala

viirya reviewed Jun 18, 2015
View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated

Copy link

Member Author

viirya Jun 18, 2015

Choose a reason for hiding this comment

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

Ah, I think this should be 1.5.0 too.

Use correct @SInCE tag.

699f48b

rxin reviewed Jun 18, 2015
View reviewed changes

Explicitly indicate sqrt value sequence.

d23e79e

rxin reviewed Jun 18, 2015
View reviewed changes

Remove unnecessary old tests.

8977e11

asfgit closed this in 3164112 Jun 18, 2015

viirya deleted the move_sqrt branch December 27, 2023 18:17

[SPARK-8363][SQL] Move sqrt to math and extend UnaryMathExpression #6823

[SPARK-8363][SQL] Move sqrt to math and extend UnaryMathExpression #6823

Uh oh!

Conversation

viirya commented Jun 15, 2015

Uh oh!

SparkQA commented Jun 15, 2015

Uh oh!

SparkQA commented Jun 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jun 15, 2015

Uh oh!

SparkQA commented Jun 15, 2015

Uh oh!

SparkQA commented Jun 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 18, 2015

Uh oh!

SparkQA commented Jun 18, 2015

Uh oh!

SparkQA commented Jun 18, 2015

Uh oh!

SparkQA commented Jun 18, 2015

Uh oh!

cloud-fan commented Jun 18, 2015

Uh oh!

rxin commented Jun 18, 2015

Uh oh!

rxin commented Jun 18, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants