This is an automated email from the ASF dual-hosted git repository.
dongjoon pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/master by this push:
new 051b1781827 [SPARK-46358][CONNECT] Simplify the condition check in the
`ResponseValidator#verifyResponse`
051b1781827 is described below
commit 051b1781827dd3a4e1e95a5354caa747ff41ae1a
Author: yangjie01 <yangji...@baidu.com>
AuthorDate: Mon Dec 11 08:53:10 2023 -0800
[SPARK-46358][CONNECT] Simplify the condition check in the
`ResponseValidator#verifyResponse`
### What changes were proposed in this pull request?
This PR has made the following refactoring to the `verifyResponse` function
in `ResponseValidator`:
1. The check condition `response.hasField(field)` is moved before getting
`value`, and only when `response.hasField(field)` is true, `value` is obtained,
which seems more in line with the existing comments.
2. Removed the `value != ""` condition check in the case match, because
only when `value.nonEmpty` is true will it enter the `if` branch, and the
condition `value.nonEmpty` has already covered the check for `value != ""`.
3. The condition check `value != id` is moved inside `case Some(id)`. After
the modification, an `IllegalStateException` will still be thrown when the id
exists and `value != id`, but `serverSideSessionId` will no longer be
reassigned when the id exists and `value == id`.
4. Removed the redundant `toString` operation when reassigning
`serverSideSessionId`, because `value` is String type.
5. Removed the No-op `case _` match, because it is unreachable code after
the above modifications.
### Why are the changes needed?
Simplify the condition check in the `verifyResponse` function
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass GitHub Actions
### Was this patch authored or co-authored using generative AI tooling?
No
Closes #44291 from LuciferYang/Simplify-ResponseValidator-verifyResponse.
Lead-authored-by: yangjie01 <yangji...@baidu.com>
Co-authored-by: YangJie <yangji...@baidu.com>
Signed-off-by: Dongjoon Hyun <dh...@apple.com>
---
.../spark/sql/connect/client/ResponseValidator.scala | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git
a/connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ResponseValidator.scala
b/connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ResponseValidator.scala
index 67f29c727ef..22c5505e7d4 100644
---
a/connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ResponseValidator.scala
+++
b/connector/connect/common/src/main/scala/org/apache/spark/sql/connect/client/ResponseValidator.scala
@@ -35,18 +35,20 @@ class ResponseValidator extends Logging {
val field =
response.getDescriptorForType.findFieldByName("server_side_session_id")
// If the field does not exist, we ignore it. New / Old message might not
contain it and this
// behavior allows us to be compatible.
- if (field != null) {
+ if (field != null && response.hasField(field)) {
val value = response.getField(field).asInstanceOf[String]
// Ignore, if the value is unset.
- if (response.hasField(field) && value != null && value.nonEmpty) {
+ if (value != null && value.nonEmpty) {
serverSideSessionId match {
- case Some(id) if value != id && value != "" =>
- throw new IllegalStateException(s"Server side session ID changed
from $id to $value")
- case _ if value != "" =>
+ case Some(id) =>
+ if (value != id) {
+ throw new IllegalStateException(
+ s"Server side session ID changed from $id to $value")
+ }
+ case _ =>
synchronized {
- serverSideSessionId = Some(value.toString)
+ serverSideSessionId = Some(value)
}
- case _ => // No-op
}
}
} else {
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@spark.apache.org
For additional commands, e-mail: commits-h...@spark.apache.org
