Re: [PR] [SPARK-46915][SQL] Simplify `UnaryMinus` `Abs` and align error class [spark]

["cloud-fan (via GitHub)" <gi...@apache.org>]

cloud-fan commented on code in PR #44942:
URL: https://github.com/apache/spark/pull/44942#discussion_r1542184876


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/arithmetic.scala:
##########
@@ -60,23 +60,15 @@ case class UnaryMinus(
 
   override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = dataType match {
     case _: DecimalType => defineCodeGen(ctx, ev, c => s"$c.unary_$$minus()")
-    case ByteType | ShortType if failOnError =>
+    case ByteType | ShortType | IntegerType | LongType if failOnError =>
+      val typeUtils = TypeUtils.getClass.getCanonicalName.stripSuffix("$")
+      val refDataType = ctx.addReferenceObj("refDataType", dataType, dataType.getClass.getName)
       nullSafeCodeGen(ctx, ev, eval => {
         val javaBoxedType = CodeGenerator.boxedType(dataType)
-        val javaType = CodeGenerator.javaType(dataType)
-        val originValue = ctx.freshName("origin")
         s"""
-           |$javaType $originValue = ($javaType)($eval);
-           |if ($originValue == $javaBoxedType.MIN_VALUE) {
-           |  throw QueryExecutionErrors.unaryMinusCauseOverflowError($originValue);
-           |}
-           |${ev.value} = ($javaType)(-($originValue));
-           """.stripMargin
-      })
-    case IntegerType | LongType if failOnError =>
-      val mathUtils = MathUtils.getClass.getCanonicalName.stripSuffix("$")
-      nullSafeCodeGen(ctx, ev, eval => {
-        s"${ev.value} = $mathUtils.negateExact($eval);"

Review Comment:
   The previous code is more efficient as it leverages the static data type information, while the new code simply passes the data type as a parameter, which means we will do data type case match per row. It will cause significant perf regression.
   
   I suggest that we add more overloads of `MathUtils.negateExact` to take byte and short, so that the generated code can still call it directly.



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