MaxGekk commented on code in PR #41314:
URL: https://github.com/apache/spark/pull/41314#discussion_r1205522488
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3130,10 +3130,18 @@ private[sql] object QueryCompilationErrors extends
QueryErrorsBase {
messageParameters = Map.empty)
}
- def invalidTimestampExprForTimeTravel(expr: Expression): Throwable = {
- new AnalysisException(
- errorClass = "_LEGACY_ERROR_TEMP_1335",
- messageParameters = Map("expr" -> expr.sql))
+ def invalidTimestampExprForTimeTravel(
+ errorClass: String,
+ expr: Expression,
+ dataType: Option[DataType] = None): Throwable = {
+ dataType match {
+ case Some(v) =>
+ new AnalysisException(errorClass = errorClass,
+ messageParameters = Map("expr" -> toSQLExpr(expr), "dataType" ->
toSQLType(v)))
+ case _ =>
+ new AnalysisException(errorClass = errorClass,
+ messageParameters = Map("expr" -> toSQLExpr(expr)))
+ }
}
Review Comment:
Could you make more readable and just overload the function:
```suggestion
def invalidTimestampExprForTimeTravel(errorClass: String, expr:
Expression): Throwable = {
new AnalysisException(
errorClass = errorClass,
messageParameters = Map("expr" -> toSQLExpr(expr)))
}
def invalidTimestampExprForTimeTravel(
errorClass: String,
expr: Expression,
dataType: DataType): Throwable = {
new AnalysisException(
errorClass = errorClass,
messageParameters = Map(
"expr" -> toSQLExpr(expr),
"dataType" -> toSQLType(dataType)))
}
```
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -1062,6 +1062,33 @@
],
"sqlState" : "42613"
},
+ "INVALID_TIME_TRAVEL_TIMESTAMP_EXPR" : {
+ "message" : [
+ "The time travel timestamp expression <expr> is invalid."
+ ],
+ "subClass" : {
+ "DATA_TYPE" : {
+ "message" : [
+ "Must be timestamp type, but got <dataType>."
+ ]
+ },
+ "IS_NULL" : {
+ "message" : [
+ "Must not be null."
Review Comment:
nit:
```suggestion
"Must not be NULL."
```
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala:
##########
@@ -2931,29 +2931,31 @@ class DataSourceV2SQLSuiteV1Filter
exception = intercept[AnalysisException] {
sql("SELECT * FROM t TIMESTAMP AS OF INTERVAL 1 DAY").collect()
},
- errorClass = "_LEGACY_ERROR_TEMP_1335",
- parameters = Map("expr" -> "INTERVAL '1' DAY"))
+ errorClass = "INVALID_TIME_TRAVEL_TIMESTAMP_EXPR.DATA_TYPE",
+ parameters = Map(
+ "expr" -> "\"INTERVAL '1' DAY\"",
+ "dataType" -> "\"INTERVAL DAY\""))
checkError(
exception = intercept[AnalysisException] {
sql("SELECT * FROM t TIMESTAMP AS OF 'abc'").collect()
},
- errorClass = "_LEGACY_ERROR_TEMP_1335",
- parameters = Map("expr" -> "'abc'"))
+ errorClass = "INVALID_TIME_TRAVEL_TIMESTAMP_EXPR.IS_NULL",
+ parameters = Map("expr" -> "\"abc\""))
Review Comment:
The error class and its message confuses slightly. `'abc'` is not NULL,
right. NULL appears as a result of casting to timestamp. Could you improve
error message, please.
--
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]
