-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-23329][SQL] Fix documentation of trigonometric functions #20618
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consine -> cosine
For the hyperbolic functions, you can just describe the argument as a "hyperbolic angle". This description is redundant with the "Returns ..." description above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not very familiar with hyperbolic functions so I followed the way this is described in java.lang.Math. But hyperbolic angle is really what this parameter actually is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the declaration of atan2 should group with the trig functions. I think that's OK to fix here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should still be moved with this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I got it. Sorry, earlier I just misunderstood it. Are you proposing to move this case class inside the file to be next to the other trigonometric function? Based on the comments it seems the functions are group based on the number of arguments. This function has 2 arguments that is why it is under // Binary math functions section. Even though this does not seem to create big cohesion inside such a group I guess we do not want to reorganize the file now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I get it now, that's my mistake. "Binary" means "two arg" and not perhaps "bitwise" or something. Leave it then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd revert the whitespace changes here, or at least only make changes that make the spacing consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use the same description here as above, for all of these functions. The one above looks better to me. Up to you whether you want to talk about the range of the return value or leave that to the Math.acos docs; it should just be consistent.
Also "cosine inverse" -> "inverse cosine", and make it clear as you do above that it's a synonym for arc cosine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
Then I will cut the part about the domains and ranges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what you mean on above. Do you mean reverting this part of the change?
How about simply @return the angle whose cosine is 'e' and refer to java.lang.Math for further details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change columnName -> colName? doesn't seem to matter, so I'd avoid changing it if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
columnName was too long and it ran into the description in the generated doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should change the name for that reason ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below -- it's not clear how ordinate and abscissa relate to the function's description. Above it's described more simply as the coordinates of a point in the plane, and I think that's more recognizable than these terms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will fix that. I borrowed that from java.lang.Math ... which sometimes is more complicated than it should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just save one line and stick to the same indentation?
arguments = """
Arguments:
* expr - number whose arc tangent is to be returned.
""",
examples = """
...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just do as below:
...
usage = """
_FUNC_(expr) - Returns hyperbolic sine of `expr`, as if computed by
`java.lang.Math._FUNC_`.
""",
...25c329b to
0a0c4ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should still be moved with this change.
| */ | ||
| def acos(e: Column): Column = withExpr { Acos(e.expr) } | ||
|
|
||
| // scalastyle:off line.size.limit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to just wrap comments rather than disable this?
| /** | ||
| * Computes the cosine inverse of the given column; the returned angle is in the range | ||
| * 0.0 through pi. | ||
| * @return angle in radians whose cosine is `columnName` as if computed by [[java.lang.Math#acos]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a big deal, but this change also makes the way this inverse function is described inconsistent with how it's described above. I think it's best to make them the same.
| /** | ||
| * Returns the angle theta from the conversion of rectangular coordinates (x, y) to | ||
| * polar coordinates (r, theta). Units in radians. | ||
| * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nit: remove this first blank line.
| /** | ||
| * Computes the cosine of the given column. | ||
| * @param columnName angle in radians | ||
| * @return cosine of the angle, as if computed by [[java.lang.Math#cos]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to line these up, especially if it makes it extend past the line limit or wrap.
|
Thanks @srowen and @HyukjinKwon for your comments so far ... |
| """) | ||
| case class Sin(child: Expression) extends UnaryMathExpression(math.sin, "SIN") | ||
|
|
||
| // scalastyle:off line.size.limit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should turn this on again
| usage = "_FUNC_(expr) - Returns the sine of `expr`.", | ||
| usage = "_FUNC_(expr) - Returns the sine of `expr`, as if computed by `java.lang.Math._FUNC_`.", | ||
| arguments = | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have the same format for it and the same instances - #20618 (comment)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I applied to all places in scope.
| """) | ||
| case class Sqrt(child: Expression) extends UnaryMathExpression(math.sqrt, "SQRT") | ||
|
|
||
| // scalastyle:off line.size.limit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto for turning it on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turned it on for all trigonometric functions.
|
|
||
| @ExpressionDescription( | ||
| usage = "_FUNC_(expr) - Returns the cotangent of `expr`.", | ||
| usage = "_FUNC_(expr) - Returns the cotangent of `expr` ," + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto for formatting
|
|
||
| @ExpressionDescription( | ||
| usage = "_FUNC_(expr) - Returns the hyperbolic tangent of `expr`.", | ||
| usage = "_FUNC_(expr) - Returns the hyperbolic tangent of `expr`, as if computed by `java.lang.Math._FUNC_`.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems line limit is reached here.
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems unrelated change. Could we just revert this back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
| case class Hypot(left: Expression, right: Expression) | ||
| extends BinaryMathExpression(math.hypot, "HYPOT") | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto for revert
|
ok to test |
|
Test build #87546 has finished for PR 20618 at commit
|
|
Test build #87547 has finished for PR 20618 at commit
|
| @ExpressionDescription( | ||
| usage = "_FUNC_(expr) - Returns the hyperbolic cosine of `expr`.", | ||
| arguments = | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean ...
arguments = """
Arguments:
* expr - hyperbolic angle.
""",
to be consistent other styles here. For example:
spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala
Lines 156 to 168 in 5683984
| arguments = """ | |
| Arguments: | |
| * str - a string expression | |
| * regexp - a string expression. The pattern string should be a Java regular expression. | |
| Since Spark 2.0, string literals (including regex patterns) are unescaped in our SQL | |
| parser. For example, to match "\abc", a regular expression for `regexp` can be | |
| "^\\abc$". | |
| There is a SQL config 'spark.sql.parser.escapedStringLiterals' that can be used to | |
| fallback to the Spark 1.6 behavior regarding string literal parsing. For example, | |
| if the config is enabled, the `regexp` that can match "\abc" is "^\abc$". | |
| """, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I will fix this.
| @ExpressionDescription( | ||
| usage = "_FUNC_(expr) - Returns the hyperbolic sine of `expr`.", | ||
| usage = """ | ||
| _FUNC_(expr) - Returns hyperbolic sine of `expr`, as if computed by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need double spaces ahead - I think the key is consistency but less invasive / targeted change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct
|
|
||
| /** | ||
| * Computes the cosine inverse of the given value; the returned angle is in the range | ||
| * 0.0 through pi. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry if I missed a discussion about this. Why don't we keep this simple description for this method rather than removing out?
If we go this way, we should update both functions.py and functions.R. If you strongly prefer this, I would like to suggest to do this separately in another PR for both places together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a valid point and I tend to agree with the conclusion to create a separate Jira for this. So far the discussion was about {{functions.scala}} but other languages should offer similar descriptions for the same thing. @srowen, can you please comment on this? I will take {{functions.scala}} out for now.
…ed functions.scala changes because it should be updated with functions.py and functions.R
HyukjinKwon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise
|
|
||
| /** | ||
| * @param e hyperbolic angle | ||
| * @return hyperbolic tangent of the given value, as if computed by [[java.lang.Math#tanh]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, but the @param and @return were fine .. I thought a doc like:
* Computes the hyperbolic tangent of the given value.
*
* @param e hyperbolic angle
* @return hyperbolic tangent computed by [[java.lang.Math#tanh]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way the description and the return value text would be redundant. For this reason it was proposed during the discussion of the issue to keep only the @return part.
| arguments = """ | ||
| Arguments: | ||
| * expr - angle in radians | ||
| """, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remote two spaces ->
* expr - angle in radians
""",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, indeed that is used elsewhere
| usage = "_FUNC_(expr) - Returns the hyperbolic cosine of `expr`.", | ||
| arguments = """ | ||
| Arguments: | ||
| * expr - hyperbolic angle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a big deal but let's remove the trailing . for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| case class Cos(child: Expression) extends UnaryMathExpression(math.cos, "COS") | ||
|
|
||
| @ExpressionDescription( | ||
| usage = "_FUNC_(expr) - Returns the hyperbolic cosine of `expr`.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we add java.lang.Math. _FUNC_ here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should
| usage = "_FUNC_(expr) - Returns the tangent of `expr`.", | ||
| usage = """ | ||
| _FUNC_(expr) - Returns the tangent of `expr`, as if computed by | ||
| `java.lang.Math._FUNC_`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems the two lines above could fix in one line.
| usage = "_FUNC_(expr) - Returns the cotangent of `expr`.", | ||
| usage = """ | ||
| _FUNC_(expr) - Returns the cotangent of `expr`, as if computed by | ||
| `1/java.lang.Math._FUNC_`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto for one line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
|
@misutoth, I am sorry. I missed #20618 (comment) and discussion in the JIRA. @srowen, I was thinking about having the same description part in other languages for now. For example, -> What do you think about not touching My only concern is to have different descriptions in other languages APIs and I am a little bit worried of when we come to fix Python's and R's. |
|
Test build #87551 has finished for PR 20618 at commit
|
|
Consistency is good. You're suggesting updating Python, R separately? If it's a big change, I could see breaking it up, but could also imagine adjusting that here. I am OK with getting this in first as I know we've iterated on this a lot. |
|
Maybe I am too much caring about this and It guess it wouldn't be a big change; however, I was thinking there might be subtle nits, like, referring Just wanted to avoid another iteration for finding a better description for both Python and R again here. (Just noticed it's his very first contribution to Spark too.). Seems fine for SQL / Scala / Java as they already use a (fully qualified) class name, for example, in case of SQL: spark-sql> DESCRIBE FUNCTION EXTENDED cos;
...
Function: cos
Class: org.apache.spark.sql.catalyst.expressions.Cos
... |
|
Test build #87554 has finished for PR 20618 at commit
|
|
Looking back I guess we can expect a couple of comments on R and Python side too, though I will target a lower number of them. :) So I am a little bit favoring moving functions.* update into a separate ticket. |
|
@HyukjinKwon I think it's fine to refer to |
|
I just thought about this again and yup I am fine. @misutoth shall we just fix R and Python ones too here? I think we could just target to fix the trigonometric functions consistently in other language's APIs too. |
|
Sure, lets do that, no problem. |
|
Was the result to add more changes to this PR or add them in another PR? |
|
cc @felixcheung (I saw you and Felix in dev mailing list). So, https://github.com/apache/spark/tree/master/R#generating-documentation does not work? |
|
@misutoth what exactly is the problem you are running into? |
|
@felixcheung, I have started a mail thread on [email protected] with title Help needed in R documentation generation because I did not feel it is directly related to this PR. Thanks for your reply on this thread already. |
|
As discussed in email R documentation is reorganized and math functions are grouped as part of SPARK-20889. Because of this grouping I dont think this change is really applicable on R. @srowen, @HyukjinKwon, @felixcheung what do you think about it? |
|
|
||
| @ExpressionDescription( | ||
| usage = "_FUNC_(expr) - Returns the sine of `expr`.", | ||
| usage = "_FUNC_(expr) - Returns the sine of `expr`, as if computed by `java.lang.Math._FUNC_`.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know the history of why the text is different here vs python/R https://github.com/apache/spark/blob/master/R/pkg/R/functions.R#L1466
I don't think the doc grouping in R is relevant - the text is there. Generally we try to match the text in scala but I don't feel strongly either way in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as the trigonometric functions concerned they should be now in sync I think.
|
Test build #87796 has finished for PR 20618 at commit
|
|
I included java.lang.Math references in functions.R |
|
Test build #87798 has finished for PR 20618 at commit
|
|
Test build #87803 has finished for PR 20618 at commit
|
|
Test build #87860 has finished for PR 20618 at commit
|
|
retest this please |
| 'radians': 'Converts an angle measured in degrees to an approximately equivalent angle ' + | ||
| 'measured in radians.', | ||
| 'degrees': """Converts an angle measured in radians to an approximately equivalent angle | ||
| measured in degrees. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a newline here. Seems doc could be broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
HyukjinKwon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise.
python/pyspark/sql/functions.py
Outdated
| measured in degrees. | ||
| :param col: angle in radians | ||
| :return: angle in degrees, as if computed by `java.lang.Math.toDegrees()`""", | ||
| 'radians': """Converts an angle measured in degrees to an approximately equivalent angle measured in radians. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this hits the line length limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I wrapped the text.
| /** | ||
| * Computes the cosine of the given value. Units in radians. | ||
| * @param e angle in radians | ||
| * @return cosine of the angle, as if computed by [[java.lang.Math#cos]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems this link is broken in Scaladoc. Let's just use `java.lang.Math.cos` in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced all of them as suggested.
| * @param yValue coordinate on y-axis | ||
| * @param xName coordinate on x-axis | ||
| * @return the <i>theta</i> component of the point | ||
| * (<i>r</i>, <i>theta</i>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit -> . Seems broken in Javadoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I have removed the non breaking spaces.
|
Test build #87883 has finished for PR 20618 at commit
|
|
Test build #87914 has finished for PR 20618 at commit
|
## What changes were proposed in this pull request? Provide more details in trigonometric function documentations. Referenced `java.lang.Math` for further details in the descriptions. ## How was this patch tested? Ran full build, checked generated documentation manually Author: Mihaly Toth <[email protected]> Closes #20618 from misutoth/trigonometric-doc. (cherry picked from commit a366b95) Signed-off-by: hyukjinkwon <[email protected]>
|
Merged to master and branch-2.3. |
## What changes were proposed in this pull request? Provide more details in trigonometric function documentations. Referenced `java.lang.Math` for further details in the descriptions. ## How was this patch tested? Ran full build, checked generated documentation manually Author: Mihaly Toth <[email protected]> Closes apache#20618 from misutoth/trigonometric-doc.
## What changes were proposed in this pull request? Provide more details in trigonometric function documentations. Referenced `java.lang.Math` for further details in the descriptions. ## How was this patch tested? Ran full build, checked generated documentation manually Author: Mihaly Toth <[email protected]> Closes apache#20618 from misutoth/trigonometric-doc. (cherry picked from commit a366b95) Signed-off-by: hyukjinkwon <[email protected]>
What changes were proposed in this pull request?
Provide more details in trigonometric function documentations. Referenced
java.lang.Mathfor further details in the descriptions.How was this patch tested?
Ran full build, checked generated documentation manually