Skip to content

[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.

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

Closed
misutoth wants to merge 10 commits into from
Closed

[SPARK-23329][SQL] Fix documentation of trigonometric functions #20618

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0a0c4ae
[SPARK-23329][SQL] Fix documentation of trigonometric functions
misutoth Feb 13, 2018
168d7b3
[SPARK-23329][SQL] Removed unnecessary aligning, reworded inverse fun…
misutoth Feb 19, 2018
2586b0f
[SPARK-23329][SQL] Fixed formatting of long lines, reverted line brea…
misutoth Feb 19, 2018
8550a27
[SPARK-23329][SQL] Make usage text consistent with other docs, revert…
misutoth Feb 20, 2018
40da998
[SPARK-23329][SQL] Formatting finetunings
misutoth Feb 20, 2018
0e4e15a
[SPARK-23329][SQL] Fix documentation of functions.R, functions.scala,…
misutoth Feb 21, 2018
fcf1338
[SPARK-23329][SQL] Fixed line length
misutoth Feb 28, 2018
2ea1f18
[SPARK-23329][SQL] Fixed line length of functions.py
misutoth Feb 28, 2018
10afda5
[SPARK-23329][SQL] Fixed python style problem
misutoth Mar 1, 2018
627e204
[SPARK-23329][SQL] Formatting fixes, changed scaladoc references
misutoth Mar 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[SPARK-23329][SQL] Fix documentation of trigonometric functions
  • Loading branch information
@misutoth
misutoth committed Feb 16, 2018
commit 0a0c4aede61a89465b474d292baf9aef773f7a80
76 changes: 64 additions & 12 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ case class Pi() extends LeafMathExpression(math.Pi, "PI")

// scalastyle:off line.size.limit
@ExpressionDescription(
usage = "_FUNC_(expr) - Returns the inverse cosine (a.k.a. arccosine) of `expr` if -1<=`expr`<=1 or NaN otherwise.",
usage = "_FUNC_(expr) - Returns the inverse cosine (a.k.a. arc cosine) of `expr`, as if computed by `java.lang.Math._FUNC_`.",
examples = """
Examples:
> SELECT _FUNC_(1);
Expand All @@ -183,7 +183,7 @@ case class Acos(child: Expression) extends UnaryMathExpression(math.acos, "ACOS"

// scalastyle:off line.size.limit
@ExpressionDescription(
usage = "_FUNC_(expr) - Returns the inverse sine (a.k.a. arcsine) the arc sin of `expr` if -1<=`expr`<=1 or NaN otherwise.",
usage = "_FUNC_(expr) - Returns the inverse sine (a.k.a. arc sine) the arc sin of `expr`, as if computed by `java.lang.Math._FUNC_`.",
examples = """
Examples:
> SELECT _FUNC_(0);
Expand All @@ -196,7 +196,7 @@ case class Asin(child: Expression) extends UnaryMathExpression(math.asin, "ASIN"

// scalastyle:off line.size.limit
@ExpressionDescription(
usage = "_FUNC_(expr) - Returns the inverse tangent (a.k.a. arctangent).",
usage = "_FUNC_(expr) - Returns the inverse tangent (a.k.a. arc tangent) of `expr`, as if computed by `java.lang.Math._FUNC_`.",
examples = """
Examples:
> SELECT _FUNC_(0);
Expand Down Expand Up @@ -252,7 +252,12 @@ case class Ceil(child: Expression) extends UnaryMathExpression(math.ceil, "CEIL"
}

@ExpressionDescription(
usage = "_FUNC_(expr) - Returns the cosine of `expr`.",
usage = "_FUNC_(expr) - Returns the cosine of `expr`, as if computed by `java.lang.Math._FUNC_`.",
arguments =
"""
Arguments:
* expr - angle in radians
""",
Copy link
Member

@HyukjinKwon HyukjinKwon Feb 20, 2018

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
  """,

Copy link
Contributor Author

@misutoth misutoth Feb 20, 2018

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

examples = """
Examples:
> SELECT _FUNC_(0);
Expand All @@ -262,6 +267,11 @@ case class Cos(child: Expression) extends UnaryMathExpression(math.cos, "COS")

@ExpressionDescription(
usage = "_FUNC_(expr) - Returns the hyperbolic cosine of `expr`.",
Copy link
Member

@HyukjinKwon HyukjinKwon Feb 20, 2018

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?

Copy link
Contributor Author

@misutoth misutoth Feb 20, 2018

Choose a reason for hiding this comment

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

Yes, we should

arguments =
"""
Copy link
Member

@HyukjinKwon HyukjinKwon Feb 20, 2018

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$".
""",

Copy link
Contributor Author

@misutoth misutoth Feb 20, 2018

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.

Arguments:
* expr - hyperbolic angle.
Copy link
Member

@HyukjinKwon HyukjinKwon Feb 20, 2018

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.

Copy link
Contributor Author

@misutoth misutoth Feb 20, 2018

Choose a reason for hiding this comment

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

ok

""",
examples = """
Examples:
> SELECT _FUNC_(0);
Expand Down Expand Up @@ -512,16 +522,27 @@ case class Rint(child: Expression) extends UnaryMathExpression(math.rint, "ROUND
case class Signum(child: Expression) extends UnaryMathExpression(math.signum, "SIGNUM")

@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_`.",
Copy link
Member

@felixcheung felixcheung Feb 28, 2018

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.

Copy link
Contributor Author

@misutoth misutoth Mar 2, 2018

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.

arguments =
"""
Copy link
Member

@HyukjinKwon HyukjinKwon Feb 19, 2018

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)?

Copy link
Contributor Author

@misutoth misutoth Feb 19, 2018

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.

Arguments:
* expr - angle in radians
""",
examples = """
Examples:
> SELECT _FUNC_(0);
0.0
""")
case class Sin(child: Expression) extends UnaryMathExpression(math.sin, "SIN")

// scalastyle:off line.size.limit
Copy link
Member

@HyukjinKwon HyukjinKwon Feb 19, 2018

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

@ExpressionDescription(
usage = "_FUNC_(expr) - Returns the hyperbolic sine of `expr`.",
usage = "_FUNC_(expr) - Returns hyperbolic sine of `expr`, as if computed by `java.lang.Math._FUNC_`.",
arguments =
"""
Arguments:
* expr - hyperbolic angle
""",
examples = """
Examples:
> SELECT _FUNC_(0);
Expand All @@ -538,8 +559,14 @@ case class Sinh(child: Expression) extends UnaryMathExpression(math.sinh, "SINH"
""")
case class Sqrt(child: Expression) extends UnaryMathExpression(math.sqrt, "SQRT")

// scalastyle:off line.size.limit
Copy link
Member

@HyukjinKwon HyukjinKwon Feb 19, 2018

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

Copy link
Contributor Author

@misutoth misutoth Feb 19, 2018

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 tangent of `expr`.",
usage = "_FUNC_(expr) - Returns the tangent of `expr`, as if computed by `java.lang.Math._FUNC_`.",
arguments =
"""
Arguments:
* expr - angle in radians
""",
examples = """
Examples:
> SELECT _FUNC_(0);
Expand All @@ -548,7 +575,13 @@ case class Sqrt(child: Expression) extends UnaryMathExpression(math.sqrt, "SQRT"
case class Tan(child: Expression) extends UnaryMathExpression(math.tan, "TAN")

@ExpressionDescription(
usage = "_FUNC_(expr) - Returns the cotangent of `expr`.",
usage = "_FUNC_(expr) - Returns the cotangent of `expr` ," +
Copy link
Member

@HyukjinKwon HyukjinKwon Feb 19, 2018

Choose a reason for hiding this comment

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

Ditto for formatting

"as if computed by `1/java.lang.Math._FUNC_`.",
arguments =
"""
Arguments:
* expr - angle in radians
""",
examples = """
Examples:
> SELECT _FUNC_(1);
Expand All @@ -562,7 +595,12 @@ case class Cot(child: Expression)
}

@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_`.",
Copy link
Member

@HyukjinKwon HyukjinKwon Feb 19, 2018

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.

arguments =
"""
Arguments:
* expr - hyperbolic angle
""",
examples = """
Examples:
> SELECT _FUNC_(0);
Expand All @@ -572,6 +610,11 @@ case class Tanh(child: Expression) extends UnaryMathExpression(math.tanh, "TANH"

@ExpressionDescription(
usage = "_FUNC_(expr) - Converts radians to degrees.",
arguments =
"""
Arguments:
* expr - angle in radians
""",
examples = """
Examples:
> SELECT _FUNC_(3.141592653589793);
Expand All @@ -583,6 +626,11 @@ case class ToDegrees(child: Expression) extends UnaryMathExpression(math.toDegre

@ExpressionDescription(
usage = "_FUNC_(expr) - Converts degrees to radians.",
arguments =
"""
Arguments:
* expr - angle in degrees
""",
examples = """
Examples:
> SELECT _FUNC_(180);
Expand Down Expand Up @@ -770,7 +818,13 @@ case class Unhex(child: Expression) extends UnaryExpression with ImplicitCastInp

// scalastyle:off line.size.limit
@ExpressionDescription(
usage = "_FUNC_(expr1, expr2) - Returns the angle in radians between the positive x-axis of a plane and the point given by the coordinates (`expr1`, `expr2`).",
Copy link
Member

@srowen srowen Feb 15, 2018

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.

Copy link
Member

@srowen srowen Feb 19, 2018

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.

Copy link
Contributor Author

@misutoth misutoth Feb 19, 2018

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?

Copy link
Member

@srowen srowen Feb 19, 2018

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.

usage = "_FUNC_(exprY, exprX) - Returns the angle in radians between the positive x-axis of a plane and the point given by the coordinates (`exprX`, `exprY`), as if computed by `java.lang.Math._FUNC_`.",
arguments =
"""
Arguments:
* exprY - coordinate on y-axis
* exprX - coordinate on x-axis
""",
examples = """
Examples:
> SELECT _FUNC_(0, 0);
Expand Down Expand Up @@ -804,7 +858,6 @@ case class Pow(left: Expression, right: Expression)
}
}


Copy link
Member

@HyukjinKwon HyukjinKwon Feb 19, 2018

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?

Copy link
Contributor Author

@misutoth misutoth Feb 19, 2018

Choose a reason for hiding this comment

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

Sure

/**
* Bitwise left shift.
*
Expand Down Expand Up @@ -916,7 +969,6 @@ case class ShiftRightUnsigned(left: Expression, right: Expression)
case class Hypot(left: Expression, right: Expression)
extends BinaryMathExpression(math.hypot, "HYPOT")


Copy link
Member

@HyukjinKwon HyukjinKwon Feb 19, 2018

Choose a reason for hiding this comment

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

Ditto for revert

/**
* Computes the logarithm of a number.
*
Expand Down
Loading