-
Notifications
You must be signed in to change notification settings - Fork 28k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-48185][SQL] Fix 'symbolic reference class is not accessible: class sun.util.calendar.ZoneInfo' #46457
Conversation
…lass sun.util.calendar.ZoneInfo'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, it looks like a safe change.
cc @LuciferYang
@@ -197,8 +197,8 @@ trait SparkDateTimeUtils { | |||
rebaseJulianToGregorianDays(julianDays) | |||
} | |||
|
|||
private val zoneInfoClassName = "sun.util.calendar.ZoneInfo" | |||
private val getOffsetsByWallHandle = { | |||
private lazy val zoneInfoClassName = "sun.util.calendar.ZoneInfo" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to use lazy
for this since zoneInfoClassName
is just a String
? 'Lazy' comes with the cost of locking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, if TimeZone.getDefault.getOffset(localMillis)
can be uniformly used to get timeZoneOffset
in the toJavaDate
function, then the use of the MethodHandle
mechanism can be avoided here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other hand, if TimeZone.getDefault.getOffset(localMillis) can be uniformly used to get timeZoneOffset in the toJavaDate function, then the use of the MethodHandle mechanism can be avoided here.
It seems that ZoneInfo.getOffset
doesn't support daylight saving transitions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, but:
- Using
TimeZone.getDefault.getOffset(localMillis)
, all tests can pass, perhaps the unit test coverage is not comprehensive, but what would be the counterexample? - Looking at the inverse function
fromJavaDate
oftoJavaDate
, it does not take into account the scenario ofZoneInfo
sql/api/src/main/scala/org/apache/spark/sql/catalyst/util/SparkDateTimeUtils.scala
Outdated
Show resolved
Hide resolved
…kDateTimeUtils.scala
…lass sun.util.calendar.ZoneInfo' ### What changes were proposed in this pull request? I met the error below while debugging UTs because of loading `sun.util.calendar.ZoneInfo` eagerly. This PR makes the relevant variables lazy. ```log Caused by: java.lang.IllegalAccessException: symbolic reference class is not accessible: class sun.util.calendar.ZoneInfo, from interface org.apache.spark.sql.catalyst.util.SparkDateTimeUtils (unnamed module 65d6b83b) at java.base/java.lang.invoke.MemberName.makeAccessException(MemberName.java:955) at java.base/java.lang.invoke.MethodHandles$Lookup.checkSymbolicClass(MethodHandles.java:3686) at java.base/java.lang.invoke.MethodHandles$Lookup.resolveOrFail(MethodHandles.java:3646) at java.base/java.lang.invoke.MethodHandles$Lookup.findVirtual(MethodHandles.java:2680) at org.apache.spark.sql.catalyst.util.SparkDateTimeUtils.$init$(SparkDateTimeUtils.scala:206) at org.apache.spark.sql.catalyst.util.DateTimeUtils$.<clinit>(DateTimeUtils.scala:41) ... 82 more ``` ### Why are the changes needed? sun.util.calendar.ZoneInfo is inaccessible in some scenarios. ### Does this PR introduce _any_ user-facing change? Yes, such errors might be delayed from backend-scheduling to job-scheduling ### How was this patch tested? I tested with idea and UT debugging locally ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#46457 from yaooqinn/SPARK-48185. Authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org>
What changes were proposed in this pull request?
I met the error below while debugging UTs because of loading
sun.util.calendar.ZoneInfo
eagerly. This PR makes the relevant variables lazy.Why are the changes needed?
sun.util.calendar.ZoneInfo is inaccessible in some scenarios.
Does this PR introduce any user-facing change?
Yes, such errors might be delayed from backend-scheduling to job-scheduling
How was this patch tested?
I tested with idea and UT debugging locally
Was this patch authored or co-authored using generative AI tooling?
no