Uh oh!
There was an error while loading. Please reload this page.
[SPARK-47706][BUILD] Bump json4s 4.0.7 - #45838
Conversation
Uh oh!
There was an error while loading. Please reload this page.
LuciferYang
commented
Apr 3, 2024
Previously, there were two unsuccessful upgrade attempts: |
I get errors when I manually run the mima check. Could you please update the |
@LuciferYang update: json4s did not leak to public API, they are false-positive results reported by Mima |
pan3793
commented
Apr 3, 2024
@LuciferYang |
There was a problem hiding this comment.
Wow.. Everything is inevitable?
There was a problem hiding this comment.
This is because the JValue package is changed, it does not mean can not read the model saved by the old Spark
def readMetadata(metadata: JValue): Metadata
There was a problem hiding this comment.
this should not be an issue
private[tree] object TreeEnsembleModel extends Logging {
object SaveLoadV1_0 {
...
}
...
}
There was a problem hiding this comment.
Is this removed or deprecated?
There was a problem hiding this comment.
No, it's there. But method JsonMethods#parse signature changed. As this is the test code, I chose the simplest way to change it.
dongjoon-hyun
left a comment
There was a problem hiding this comment.
It looks like inevitable change if we want to use the latest one.
+1 for Apache Spark 4.0.0.
WDYT, cc @HyukjinKwon , @cloud-fan , @gengliangwang ?
gengliangwang
commented
Apr 6, 2024
Since there are many connectors/libraries that depend on Spark, I am +0 on this one. |
cloud-fan
commented
Apr 8, 2024
does json4s release notes mention anything about backward compatibility? We have persistent event logs in JSON format that may need to be consumed by new version of Spark history server. |
pan3793
commented
Apr 8, 2024
@cloud-fan seems json4s does not have release note. the API binary incompatible does not affect JSON serialize/deserialize, the case of parsing event log generated by the previous Spark version is already covered by Spark UT. |
There was a problem hiding this comment.
I noticed that DefaultParamsReader's scope is private[ml], but Mima still complains, is it expected? cc @LuciferYang
private[ml] class DefaultParamsReader
There was a problem hiding this comment.
Could you file a JIRA issue for this, @pan3793 ? It seems that we had better fix that first.
dongjoon-hyun
commented
Apr 9, 2024
To @pan3793 , for MIMA issue, I made the following PR to fix it. |
There was a problem hiding this comment.
it's weird
trait Row {
...
private[sql] def jsonValue: JValue
...
}
abstract class MutableAggregationBuffer extends Row {
...
}
There was a problem hiding this comment.
this should not be a real issue
object DataType {
private object JSortedObject {
...
}
...
}
pan3793
commented
Apr 9, 2024
@dongjoon-hyun@cloud-fan@gengliangwang@LuciferYang I updated MimaExcludes, now it looks good. |
dongjoon-hyun
commented
Apr 9, 2024
Thank you for rebasing. |
pan3793
commented
Apr 10, 2024
rebased on master |
LuciferYang
commented
Apr 10, 2024
If there are no compatibility issues, this is great. It seems that json4s 4.x supports Scala 3, which further increases the possibility of us upgrading to Scala 3 in the future. If this upgrade is feasible, we can close https://issues.apache.org/jira/browse/SPARK-44172 at the same time. |
LuciferYang
left a comment
There was a problem hiding this comment.
+1, LGTM
Thanks @pan3793 and @dongjoon-hyun
dongjoon-hyun
commented
Apr 10, 2024
Could you re-trigger the failed test pipeline, @pan3793 ? |
pan3793
commented
Apr 10, 2024
@LuciferYang@dongjoon-hyun The failed test is known flaky case, I re-triggered CI, thanks for your approval and help with Mima |
dongjoon-hyun
commented
Apr 10, 2024
Thank you, @pan3793 and all. |
What changes were proposed in this pull request?
Bump json4s from 3.7.0-M11 to 4.0.7
Why are the changes needed?
4.0.7 is the latest stable version of json4s.
https://mvnrepository.com/artifact/org.json4s/json4s-jackson
Does this PR introduce any user-facing change?
No, all Mima complaints are private API.
How was this patch tested?
Pass GHA.
Was this patch authored or co-authored using generative AI tooling?
No