viirya commented on a change in pull request #34729:
URL: https://github.com/apache/spark/pull/34729#discussion_r769894308
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
##########
@@ -249,9 +249,9 @@ case class Cbrt(child: Expression) extends
UnaryMathExpression(math.cbrt, "CBRT"
""",
since = "1.4.0",
group = "math_funcs")
-case class Ceil(child: Expression) extends UnaryMathExpression(math.ceil,
"CEIL") {
+ case class Ceil(child: Expression) extends UnaryMathExpression(math.ceil,
"CEIL") {
Review comment:
Seems there is redundant space?
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
##########
@@ -249,9 +249,9 @@ case class Cbrt(child: Expression) extends
UnaryMathExpression(math.cbrt, "CBRT"
""",
since = "1.4.0",
group = "math_funcs")
-case class Ceil(child: Expression) extends UnaryMathExpression(math.ceil,
"CEIL") {
+ case class Ceil(child: Expression) extends UnaryMathExpression(math.ceil,
"CEIL") {
override def dataType: DataType = child.dataType match {
- case dt @ DecimalType.Fixed(_, 0) => dt
+ case [email protected](_, 0) => dt
Review comment:
We should keep original style.
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
##########
@@ -457,11 +474,11 @@ case class Expm1(child: Expression) extends
UnaryMathExpression(StrictMath.expm1
> SELECT _FUNC_(5);
5
""",
- since = "1.4.0",
+ since = "3.3.0",
Review comment:
I think we don't need to change this.
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
##########
@@ -275,10 +275,27 @@ case class Ceil(child: Expression) extends
UnaryMathExpression(math.ceil, "CEIL"
case _ => defineCodeGen(ctx, ev, c =>
s"(long)(java.lang.Math.${funcName}($c))")
}
}
-
Review comment:
Unnecessary change.
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
##########
@@ -249,9 +249,9 @@ case class Cbrt(child: Expression) extends
UnaryMathExpression(math.cbrt, "CBRT"
""",
since = "1.4.0",
group = "math_funcs")
-case class Ceil(child: Expression) extends UnaryMathExpression(math.ceil,
"CEIL") {
+ case class Ceil(child: Expression) extends UnaryMathExpression(math.ceil,
"CEIL") {
override def dataType: DataType = child.dataType match {
Review comment:
Do you think will it possibly that `scale` to be not only a literal in
the future? Currently looks like the way to keep `ceil(c_double)` and
`ceil(c_double, 0)` have consistent data type is to check `scale` value before
constructing `Ceil`/`RoundCeil` expr. Seems if `scale` isn't a literal, we
cannot do this. If we are pretty sure `scale` cannot be non-literal in the
future, then it is fine.
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
##########
@@ -457,11 +474,11 @@ case class Expm1(child: Expression) extends
UnaryMathExpression(StrictMath.expm1
> SELECT _FUNC_(5);
5
""",
- since = "1.4.0",
+ since = "3.3.0",
group = "math_funcs")
case class Floor(child: Expression) extends UnaryMathExpression(math.floor,
"FLOOR") {
override def dataType: DataType = child.dataType match {
- case dt @ DecimalType.Fixed(_, 0) => dt
+ case [email protected](_, 0) => dt
Review comment:
ditto
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/functions.scala
##########
@@ -1875,15 +1883,23 @@ object functions {
def factorial(e: Column): Column = withExpr { Factorial(e.expr) }
/**
- * Computes the floor of the given value.
+// * Computes the floor of the given value of `e` to `scale` decimal places.
+// *
+// * @group math_funcs
+// * @since 3.3.0
+// */
Review comment:
The comment style looks incorrect.
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
##########
@@ -275,10 +275,27 @@ case class Ceil(child: Expression) extends
UnaryMathExpression(math.ceil, "CEIL"
case _ => defineCodeGen(ctx, ev, c =>
s"(long)(java.lang.Math.${funcName}($c))")
}
}
-
override protected def withNewChildInternal(newChild: Expression): Ceil =
copy(child = newChild)
}
+object Ceil {
+ def apply(child: Expression, scale: Expression): Expression = {
+ if (scale.eval(EmptyRow).asInstanceOf[Int] == 0) {
Review comment:
Can `scale` possibly be evaluated to null and get a NPE here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
