[GitHub] [spark] MaxGekk commented on a diff in pull request #38588: [SPARK-41086][SQL] Consolidate SecondArgumentXXX error to INVALID_PARAMETER_VALUE

[GitBox]

MaxGekk commented on code in PR #38588:
URL: https://github.com/apache/spark/pull/38588#discussion_r1018715875


##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -2082,10 +2084,12 @@ private[sql] object QueryCompilationErrors extends 
QueryErrorsBase {
       messageParameters = Map("fields" -> fields))
   }
 
-  def secondArgumentInFunctionIsNotBooleanLiteralError(funcName: String): 
Throwable = {
+  def secondArgumentInFunctionIsNotBooleanLiteralError(
+      funcName: String, parameter: String): Throwable = {
     new AnalysisException(
-      errorClass = "_LEGACY_ERROR_TEMP_1210",
-      messageParameters = Map("funcName" -> funcName))
+      errorClass = "INVALID_PARAMETER_VALUE",
+      messageParameters =
+        Map("parameter" -> parameter, "functionName" -> funcName, "expected" 
-> "Boolean"))

Review Comment:
   Please, quote parameters.



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -1085,10 +1085,11 @@ private[sql] object QueryCompilationErrors extends 
QueryErrorsBase {
       messageParameters = Map("clz" -> clz.toString))
   }
 
-  def secondArgumentNotDoubleLiteralError(): Throwable = {
+  def secondArgumentNotDoubleLiteralError(functionName: String, parameter: 
String): Throwable = {
     new AnalysisException(
-      errorClass = "_LEGACY_ERROR_TEMP_1104",
-      messageParameters = Map.empty)
+      errorClass = "INVALID_PARAMETER_VALUE",
+      messageParameters =
+        Map("parameter" -> parameter, "functionName" -> functionName, 
"expected" -> "Double"))

Review Comment:
   The error message parameters should be quoted:
   functionName -> toSQLId(functionName)
   Double -> toSQLType(DoubleType)



##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -2034,11 +2035,12 @@ private[sql] object QueryCompilationErrors extends 
QueryErrorsBase {
   }
 
   def secondArgumentOfFunctionIsNotIntegerError(
-      function: String, e: NumberFormatException): Throwable = {
+      function: String, parameter: String, e: NumberFormatException): 
Throwable = {
     // The second argument of {function} function needs to be an integer
     new AnalysisException(
-      errorClass = "SECOND_FUNCTION_ARGUMENT_NOT_INTEGER",
-      messageParameters = Map("functionName" -> function),
+      errorClass = "INVALID_PARAMETER_VALUE",
+      messageParameters =
+        Map("parameter" -> parameter, "functionName" -> function, "expected" 
-> "Integer"),

Review Comment:
   The same comments as for secondArgumentNotDoubleLiteralError



##########
core/src/main/resources/error/error-classes.json:
##########
@@ -2109,11 +2103,6 @@
       "Unsupported component type <clz> in arrays."
     ]
   },
-  "_LEGACY_ERROR_TEMP_1104" : {
-    "message" : [
-      "The second argument should be a double literal."

Review Comment:
   This has additional requirements:
   1. Be a literal
   2. and it can be a decimal literal
   
   see
   ```scala
     def validateDoubleLiteral(exp: Expression): Double = exp match {
       case Literal(d: Double, DoubleType) => d
       case Literal(dec: Decimal, _) => dec.toDouble
       case _ =>
         throw QueryCompilationErrors.secondArgumentNotDoubleLiteralError
     }
   ```



##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/aggregate/FirstLastTestSuite.scala:
##########
@@ -112,14 +112,14 @@ class FirstLastTestSuite extends SparkFunSuite {
     val msg1 = intercept[AnalysisException] {
       new First(input, Literal(1, IntegerType))
     }.getMessage
-    assert(msg1.contains("The second argument in first should be a boolean 
literal"))
+    assert(msg1.contains("INVALID_PARAMETER_VALUE"))

Review Comment:
   Use checkError() to make tests independent from error messages. In this way, 
tech editors can change error message templates w/ modifying internal Spark 
tests.



-- 
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-unsubscr...@spark.apache.org

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


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