[GitHub] [spark] viirya commented on a change in pull request #34729: [SPARK-37475][SQL] Add scale parameter to floor and ceil functions

[GitBox <gi...@apache.org>]

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 dt@DecimalType.Fixed(_, 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 dt@DecimalType.Fixed(_, 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: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org