[SPARK-20751][SQL] Add built-in SQL Function - COT #17999

wangyum · 2017-05-16T10:47:20Z

What changes were proposed in this pull request?

Add built-in SQL Function - COT.

How was this patch tested?

unit tests

SparkQA · 2017-05-16T17:35:13Z

Test build #76959 has finished for PR 17999 at commit d30484c.

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

gatorsmile · 2017-05-16T17:50:59Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

+  """)
+case class Cot(child: Expression)
+  extends UnaryMathExpression((x: Double) => 1 / math.tan(x), "COT") {
+  override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {


Please use defineCodeGen

gatorsmile · 2017-05-16T17:51:52Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

+  override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
+    nullSafeCodeGen(ctx, ev, c =>
+      s"""
+        if (${ev.value} == null) {


Internally, it is already nullSafeCodeGen, and thus, this is not needed.

gatorsmile · 2017-05-16T17:53:27Z

Thanks for working on it. You need to add it to FunctionRegistry and add the SQL-related test cases to operators.sql. Also covers all the edge cases. For example, 0 that should trigger infinity, null, and so on.

maropu · 2017-05-16T19:57:44Z

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

+   * @since 2.2.0
+   */
+  def cot(colName: String): Column = cot(Column(colName))
+


I'm not sure though, do we strictly need to add this function in this file? If we add this in FunctionRegistry, you know we can use this thru selectExpr. ISTM this file seems to include frequently-used functions only. cc: @gatorsmile

Sure. Let us keep this file untouched in this PR.

maropu · 2017-05-16T19:57:58Z

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

+  /**
+   * Computes the cotangent of the given column.
+   *
+   * @group math_funcs


nit: 2.3.0?

SparkQA · 2017-05-17T19:30:04Z

Test build #77025 has finished for PR 17999 at commit 3782512.

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

maropu · 2017-05-17T21:29:28Z

As @gatorsmile said, you need to add tests in operators.sql (https://github.com/apache/spark/blob/master/sql/core/src/test/resources/sql-tests/inputs/operators.sql). See SQLQueryTestSuite for these test cases.

SparkQA · 2017-05-18T05:17:43Z

Test build #77041 has started for PR 17999 at commit c80c184.

maropu · 2017-05-18T15:25:50Z

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

+   * @since 2.3.0
+   */
+  def cot(colName: String): Column = cot(Column(colName))
+


We do not add these functions here in this pr. See: #17999 (diff)

SparkQA · 2017-05-18T16:17:39Z

Test build #77057 has started for PR 17999 at commit 3bd1e1e.

gatorsmile · 2017-05-18T16:47:31Z

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

+   *
+   * @group math_funcs
+   * @since 2.3.0
+   */


Let us remove this too. Thanks!

gatorsmile · 2017-05-18T16:48:20Z

sql/core/src/test/scala/org/apache/spark/sql/MathFunctionsSuite.scala

+  test("cot") {
+    testOneToOneMathFunction(cot, (d: Double) => 1 / math.tan(d))
+    checkAnswer(
+      sql(s"SELECT cot(null), cot(0), cot(-1), cot(${Double.MaxValue})"),


Could you move these tests to operators.sql?

gatorsmile · 2017-05-18T16:49:07Z

sql/core/src/test/resources/sql-tests/inputs/operators.sql

 select 2 * 5;
 select 5 % 3;
 select pmod(-7, 3);
+select cot(1);


Could you move this to the end of this .sql file? It can reduce the line of code changes.

gatorsmile · 2017-05-18T16:55:59Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

+    defineCodeGen(ctx, ev, c =>
+      s"""
+        ${ev.value} = 1 / java.lang.Math.tan($c);
+      """


Since it fits one line, could you change it to?

defineCodeGen(ctx, ev, c => s"${ev.value} = 1 / java.lang.Math.tan($c);")

gatorsmile · 2017-05-18T16:59:05Z

Thanks for working on it!

LGTM except a few minor comments.

SparkQA · 2017-05-19T01:18:18Z

Test build #77067 has finished for PR 17999 at commit ea10dee.

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

gatorsmile · 2017-05-19T16:40:45Z

Thanks! Merging to master.

rxin · 2017-05-19T23:29:49Z

hmnmm

seems like we should be following how we test tan, cos, etc in MathExpressionsSuite?

## What changes were proposed in this pull request? Add cot test in MathExpressionsSuite as apache#17999 (comment). ## How was this patch tested? unit tests Author: Yuming Wang <[email protected]> Closes apache#18039 from wangyum/SPARK-20751-test.

## What changes were proposed in this pull request? Add built-in SQL Function - COT. ## How was this patch tested? unit tests Author: Yuming Wang <[email protected]> Closes apache#17999 from wangyum/SPARK-20751.

## What changes were proposed in this pull request? Add cot test in MathExpressionsSuite as apache#17999 (comment). ## How was this patch tested? unit tests Author: Yuming Wang <[email protected]> Closes apache#18039 from wangyum/SPARK-20751-test.

### What changes were proposed in this pull request? Add new built-in SQL functions: secant and cosecant, and add them as Scala and Python functions. ### Why are the changes needed? Cotangent has been supported in Spark SQL but Secant and Cosecant are missing though I believe they can be used as much as cot. Related Links: [SPARK-20751](#17999) [SPARK-36660](#33906) ### Does this PR introduce _any_ user-facing change? Yes, users can now use these functions. ### How was this patch tested? Unit tests Closes #33988 from yutoacts/SPARK-36683. Authored-by: Yuto Akutsu <[email protected]> Signed-off-by: Kousuke Saruta <[email protected]>

### What changes were proposed in this pull request? Add new built-in SQL functions: secant and cosecant, and add them as Scala and Python functions. ### Why are the changes needed? Cotangent has been supported in Spark SQL but Secant and Cosecant are missing though I believe they can be used as much as cot. Related Links: [SPARK-20751](apache/spark#17999) [SPARK-36660](apache/spark#33906) ### Does this PR introduce _any_ user-facing change? Yes, users can now use these functions. ### How was this patch tested? Unit tests Closes #33988 from yutoacts/SPARK-36683. Authored-by: Yuto Akutsu <[email protected]> Signed-off-by: Kousuke Saruta <[email protected]>

Add cotangent function.

d30484c

gatorsmile reviewed May 16, 2017

View reviewed changes

maropu reviewed May 16, 2017

View reviewed changes

Registry function, add edge test cases.

3782512

Add test in operators.sql

c80c184

maropu reviewed May 18, 2017

View reviewed changes

Remove cot(colName: String)

3bd1e1e

gatorsmile reviewed May 18, 2017

View reviewed changes

Move cot tests to operators.sql.

ea10dee

asfgit closed this in bff021d May 19, 2017

wangyum mentioned this pull request May 20, 2017

[SPARK-20751][SQL][FOLLOWUP] Add cot test in MathExpressionsSuite #18039

Closed

wangyum deleted the SPARK-20751 branch October 8, 2019 04:24

yutoacts mentioned this pull request Sep 14, 2021

[SPARK-36683][SQL] Add new built-in SQL functions: SEC and CSC #33988

Closed

[SPARK-20751][SQL] Add built-in SQL Function - COT #17999

[SPARK-20751][SQL] Add built-in SQL Function - COT #17999

Uh oh!

Conversation

wangyum commented May 16, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented May 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented May 16, 2017

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 May 17, 2017

Uh oh!

maropu commented May 17, 2017

Uh oh!

SparkQA commented May 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 18, 2017

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!

gatorsmile commented May 18, 2017

Uh oh!

SparkQA commented May 19, 2017

Uh oh!

gatorsmile commented May 19, 2017

Uh oh!

rxin commented May 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants