You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "JoshRosen (via GitHub)" <gi...@apache.org> on 2023/02/11 02:09:15 UTC
[GitHub] [spark] JoshRosen opened a new pull request, #39973: [SPARK-42403][CORE] JsonProtocol should handle null Json strings
JoshRosen opened a new pull request, #39973:
URL: https://github.com/apache/spark/pull/39973
### What changes were proposed in this pull request?
This PR fixes a regression introduced in #39972 which broke JsonProtocol's ability to parse `null` string values: the old Json4S-based parser would correctly parse null literals, whereas the new code rejects them via an overly-strict type check.
This PR solves this problem by relaxing the type checking in `extractString` so that `null` literals in JSON can be parsed as `null` strings.
### Why are the changes needed?
Fix a regression which prevents the history server from parsing certain types of event logs which contain null strings, including stacktraces containing generated code frames and ExceptionFailure messages where the exception message is `null`.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Added new unit test in JsonProtocolSuite.
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #39973: [SPARK-42403][CORE] JsonProtocol should handle null JSON strings
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #39973:
URL: https://github.com/apache/spark/pull/39973#issuecomment-1426579436
Hey, @JoshRosen . I didn't introduce a regression. Could you fix your PR description?
> This PR fixes a regression introduced in https://github.com/apache/spark/pull/39972
--
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
[GitHub] [spark] JoshRosen commented on pull request #39973: [SPARK-42403][CORE] JsonProtocol should handle null JSON strings
Posted by "JoshRosen (via GitHub)" <gi...@apache.org>.
JoshRosen commented on PR #39973:
URL: https://github.com/apache/spark/pull/39973#issuecomment-1426585117
Whoops, thanks for fixing that PR number mixup in the description 👍
--
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
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #39973: [SPARK-42403][CORE] JsonProtocol should handle null JSON strings
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #39973:
URL: https://github.com/apache/spark/pull/39973#discussion_r1103485692
##########
core/src/test/scala/org/apache/spark/util/JsonProtocolSuite.scala:
##########
@@ -777,6 +777,38 @@ class JsonProtocolSuite extends SparkFunSuite {
|}""".stripMargin
assert(JsonProtocol.sparkEventFromJson(unknownFieldsJson) === expected)
}
+
+ test("SPARK-42403: properly handle null string values") {
Review Comment:
Thank you for adding this test case.
--
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
[GitHub] [spark] cloud-fan commented on pull request #39973: [SPARK-42403][CORE] JsonProtocol should handle null JSON strings
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #39973:
URL: https://github.com/apache/spark/pull/39973#issuecomment-1427337069
late lgtm, thanks for the fix!
--
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
[GitHub] [spark] dongjoon-hyun closed pull request #39973: [SPARK-42403][CORE] JsonProtocol should handle null JSON strings
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #39973: [SPARK-42403][CORE] JsonProtocol should handle null JSON strings
URL: https://github.com/apache/spark/pull/39973
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #39973: [SPARK-42403][CORE] JsonProtocol should handle null JSON strings
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #39973:
URL: https://github.com/apache/spark/pull/39973#issuecomment-1426583242
Never mind. I fixed the PR description.
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #39973: [SPARK-42403][CORE] JsonProtocol should handle null JSON strings
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #39973:
URL: https://github.com/apache/spark/pull/39973#issuecomment-1426579655
This sounds like misleading, @JoshRosen . Could you point your PR which introduce this regression ?
> This PR fixes a regression introduced in https://github.com/apache/spark/pull/39972
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #39973: [SPARK-42403][CORE] JsonProtocol should handle null JSON strings
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #39973:
URL: https://github.com/apache/spark/pull/39973#issuecomment-1426580053
This regression was introduced at https://github.com/apache/spark/pull/36885 .
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #39973: [SPARK-42403][CORE] JsonProtocol should handle null JSON strings
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #39973:
URL: https://github.com/apache/spark/pull/39973#issuecomment-1426628909
Thank you, @JoshRosen and @mridulm .
Merged to master/3.4.
--
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