You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "stefanbuk-db (via GitHub)" <gi...@apache.org> on 2024/03/11 13:58:08 UTC
[PR] [SPARK-47343][SQL] Fix NPE when `sqlString` variable value is null string in execute immediate [spark]
stefanbuk-db opened a new pull request, #45462:
URL: https://github.com/apache/spark/pull/45462
### What changes were proposed in this pull request?
In this PR I propose a change to add new error class NULL_QUERY_STRING_EXECUTE_IMMEDIATE that is thrown when sqlString variable is a null string in execute immediate query. Null pointer exception was thrown before this change.
### Why are the changes needed?
To ensure that user facing error is as clear as possible and to avoid NPE for possible user input.
### Does this PR introduce _any_ user-facing change?
Yes. This PR introduces a new error class that has an user facing description.
### How was this patch tested?
By running an "EXEC IMMEDIATE - Null string as sqlString parameter" test from AnalysisErrorSuite.
### Was this patch authored or co-authored using generative AI tooling?
No
--
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
Re: [PR] [SPARK-47343][SQL] Fix NPE when `sqlString` variable value is null string in execute immediate [spark]
Posted by "milastdbx (via GitHub)" <gi...@apache.org>.
milastdbx commented on code in PR #45462:
URL: https://github.com/apache/spark/pull/45462#discussion_r1521059360
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/executeImmediate.scala:
##########
@@ -88,10 +88,16 @@ class SubstituteExecuteImmediate(val catalogManager: CatalogManager)
throw QueryCompilationErrors.invalidExecuteImmediateVariableType(varReference.dataType)
}
+ val varReferenceValue = varReference.eval(null)
+
+ if (varReferenceValue == null) {
+ throw QueryCompilationErrors.nullSQLStringExecuteImmediate(u.name)
+ }
+
// Call eval with null value passed instead of a row.
Review Comment:
you forgot to move comment above `eval`
--
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
Re: [PR] [SPARK-47343][SQL] Fix NPE when `sqlString` variable value is null string in execute immediate [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #45462:
URL: https://github.com/apache/spark/pull/45462#discussion_r1523678372
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3004,6 +3004,12 @@
],
"sqlState" : "2200E"
},
+ "NULL_QUERY_STRING_EXECUTE_IMMEDIATE" : {
+ "message" : [
+ "Execute immediate requires a non-null variable as the query string, but the provided variable <varName> is null."
+ ],
+ "sqlState" : "42K09"
Review Comment:
22004 it is
--
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
Re: [PR] [SPARK-47343][SQL] Fix NPE when `sqlString` variable value is null string in execute immediate [spark]
Posted by "milastdbx (via GitHub)" <gi...@apache.org>.
milastdbx commented on code in PR #45462:
URL: https://github.com/apache/spark/pull/45462#discussion_r1519779866
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3004,6 +3004,12 @@
],
"sqlState" : "2200E"
},
+ "NULL_QUERY_STRING_EXECUTE_IMMEDIATE" : {
+ "message" : [
+ "SQLQuery string should not be null."
Review Comment:
perhaps something like: `Execute immediate requires non-null value for sqlString variable, but provided <varName> is null`
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/executeImmediate.scala:
##########
@@ -88,6 +88,10 @@ class SubstituteExecuteImmediate(val catalogManager: CatalogManager)
throw QueryCompilationErrors.invalidExecuteImmediateVariableType(varReference.dataType)
}
+ if (varReference.eval(null) == null) {
Review Comment:
consider storing `varReference` in variable so we don't evalulate multiple times
--
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
Re: [PR] [SPARK-47343][SQL] Fix NPE when `sqlString` variable value is null string in execute immediate [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #45462:
URL: https://github.com/apache/spark/pull/45462#issuecomment-1991435342
+1, LGTM. Merging to master.
Thank you, @stefanbuk-db and @milastdbx for review.
--
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
Re: [PR] [SPARK-47343][SQL] Fix NPE when `sqlString` variable value is null string in execute immediate [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #45462:
URL: https://github.com/apache/spark/pull/45462#discussion_r1520631768
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3004,6 +3004,12 @@
],
"sqlState" : "2200E"
},
+ "NULL_QUERY_STRING_EXECUTE_IMMEDIATE" : {
+ "message" : [
+ "Execute immediate requires non-null value for sqlString variable, but provided <varName> is null."
Review Comment:
Is it clear from user perspective what's `sqlString` is?
How about:
```suggestion
"Execute immediate requires a non-null variable as its argument, but the provided variable <varName> is null."
```
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3004,6 +3004,12 @@
],
"sqlState" : "2200E"
},
+ "NULL_QUERY_STRING_EXECUTE_IMMEDIATE" : {
+ "message" : [
+ "Execute immediate requires non-null value for sqlString variable, but provided <varName> is null."
Review Comment:
or
```suggestion
"Execute immediate requires a non-null variable as the query string, but the provided variable <varName> is null."
```
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryCompilationErrors.scala:
##########
@@ -3954,6 +3954,12 @@ private[sql] object QueryCompilationErrors extends QueryErrorsBase with Compilat
messageParameters = Map("varType" -> toSQLType(dataType)))
}
+ def nullSQLStringExecuteImmediate(varName: String): Throwable = {
+ throw new AnalysisException(
+ errorClass = "NULL_QUERY_STRING_EXECUTE_IMMEDIATE",
+ messageParameters = Map("varName" -> varName))
Review Comment:
Please, quote `varName` by `toSQLId` since it is an identifier, I guess. Isn't it?
--
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
Re: [PR] [SPARK-47343][SQL] Fix NPE when `sqlString` variable value is null string in execute immediate [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #45462:
URL: https://github.com/apache/spark/pull/45462#issuecomment-1991433598
The failed test:
```
[info] OracleIntegrationSuite:
...
[info] Cause: oracle.net.ns.NetException: ORA-12541: Cannot connect. No listener at host 10.1.1.146 port 46609.
```
is not related to the changes, and it passed successfully on the previous commit.
--
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
Re: [PR] [SPARK-47343][SQL] Fix NPE when `sqlString` variable value is null string in execute immediate [spark]
Posted by "milastdbx (via GitHub)" <gi...@apache.org>.
milastdbx commented on code in PR #45462:
URL: https://github.com/apache/spark/pull/45462#discussion_r1521058729
##########
common/utils/src/main/resources/error/error-classes.json:
##########
@@ -3004,6 +3004,12 @@
],
"sqlState" : "2200E"
},
+ "NULL_QUERY_STRING_EXECUTE_IMMEDIATE" : {
+ "message" : [
+ "Execute immediate requires a non-null variable as the query string, but the provided variable <varName> is null."
+ ],
+ "sqlState" : "42K09"
Review Comment:
`22004` would be a correct code.
@srielau to confirm
--
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
Re: [PR] [SPARK-47343][SQL] Fix NPE when `sqlString` variable value is null string in execute immediate [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #45462: [SPARK-47343][SQL] Fix NPE when `sqlString` variable value is null string in execute immediate
URL: https://github.com/apache/spark/pull/45462
--
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