Skip to content

Conversation

@yutoacts
Copy link
Contributor

@yutoacts yutoacts commented Sep 3, 2021

What changes were proposed in this pull request?

Add cotangent support by Dataframe operations (e.g. df.select(cot($"col"))).

Why are the changes needed?

Cotangent has been supported by Spark SQL since 2.3.0 but it cannot be called by Dataframe operations.

Does this PR introduce any user-facing change?

Yes, users can now call the cotangent function by Dataframe operations.

How was this patch tested?

manual test.

@yutoacts
Copy link
Contributor Author

yutoacts commented Sep 3, 2021

cc @wangyum

@wangyum
Copy link
Member

wangyum commented Sep 3, 2021

cc @HyukjinKwon

* @group math_funcs
* @since 3.2.0
*/
def cot(e: Column): Column = withExpr { Cot(e.expr) }
Copy link
Member

Choose a reason for hiding this comment

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

Let's only add this one (Column only).

Copy link
Contributor Author

@yutoacts yutoacts Sep 3, 2021

Choose a reason for hiding this comment

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

Thanks for the review. I saw other trig functions accept both Column and String(columnName). Should we not use columnName for the parameter?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, read the guides on the top of f functions.scala

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, thank you I just removed it.

@HyukjinKwon HyukjinKwon changed the title [SPARK-36660][SQL] Support COT by Dataframe operations [SPARK-36660][SQL] Add cot as Scala and Python functions Sep 3, 2021
@HyukjinKwon
Copy link
Member

ok to test

@HyukjinKwon
Copy link
Member

I am okay with this.
cc @zero323, @WeichenXu123 and @srowen FYI

@SparkQA
Copy link

SparkQA commented Sep 3, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47469/

@yutoacts
Copy link
Contributor Author

yutoacts commented Sep 3, 2021

BTW noticed that cosecant and secant are missing. Though they are just the reciprocals of sin and cos, I feel a bit odd that only cot is supported.

@SparkQA
Copy link

SparkQA commented Sep 3, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47469/

@SparkQA
Copy link

SparkQA commented Sep 3, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47473/

@SparkQA
Copy link

SparkQA commented Sep 3, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47473/

@zero323
Copy link
Member

zero323 commented Sep 3, 2021

The arguments here are the same as for SPARK-33061 (#29938). so if we've added those, this one should be fine as well.

@SparkQA
Copy link

SparkQA commented Sep 3, 2021

Test build #142969 has finished for PR 33906 at commit ee170d6.

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

@SparkQA
Copy link

SparkQA commented Sep 3, 2021

Test build #142972 has finished for PR 33906 at commit c142527.

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

* @group math_funcs
* @since 3.2.0
*/
def cot(columnName: String): Column = cot(Column(columnName))
Copy link
Member

Choose a reason for hiding this comment

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

Mind removing this one? Looks fine otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the reminder. I just removed it.

@SparkQA
Copy link

SparkQA commented Sep 6, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47508/

@SparkQA
Copy link

SparkQA commented Sep 6, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47508/

@SparkQA
Copy link

SparkQA commented Sep 6, 2021

Test build #143006 has finished for PR 33906 at commit a4ec1e1.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member

Merged to master.

@yutoacts
Copy link
Contributor Author

yutoacts commented Sep 7, 2021

I mentioned before but how about implementing SEC/CSC? I believe they can be used as much as COT.

@HyukjinKwon
Copy link
Member

I am fine with them.

return _invoke_function_over_column("cosh", col)


def cot(col):
Copy link
Member

Choose a reason for hiding this comment

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

actually you should also add them in functions.R too. ref: aeb45df

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I forgot about that. I'm pushing the follow-up PR soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

this is fine. usually SparkR one is done separately in a separate JIRA.

@yutoacts
Copy link
Contributor Author

yutoacts commented Sep 7, 2021

Thank you. I'll post the new ticket and start working on them.

HyukjinKwon pushed a commit that referenced this pull request Sep 8, 2021
### What changes were proposed in this pull request?

Add cotangent as an R function.

### Why are the changes needed?

My previous PR (#33906) missed R support.

### Does this PR introduce _any_ user-facing change?

Yes, users can now call the cotangent function as an R function.

### How was this patch tested?

unit tests.

Closes #33925 from yutoacts/SPARK-36660.

Lead-authored-by: Yuto Akutsu <[email protected]>
Co-authored-by: Yuto Akutsu <[email protected]>
Signed-off-by: Hyukjin Kwon <[email protected]>
@sarutak
Copy link
Member

sarutak commented Sep 14, 2021

We need to change python/docs/source/reference/pyspark.sql.rst for cot right?

@HyukjinKwon
Copy link
Member

Yup

@yutoacts
Copy link
Contributor Author

Thank you, I just added to it.

HyukjinKwon pushed a commit that referenced this pull request Sep 15, 2021
### What changes were proposed in this pull request?

Added cot to pyspark.sql.rst (follow-up)

### Why are the changes needed?

[My previous PR](#33906) was missing it.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

manual check

Closes #34002 from yutoacts/SPARK-36660.

Authored-by: Yuto Akutsu <[email protected]>
Signed-off-by: Hyukjin Kwon <[email protected]>
sarutak pushed a commit that referenced this pull request Sep 20, 2021
### 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]>
a0x8o added a commit to a0x8o/spark that referenced this pull request Sep 20, 2021
### 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants