You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "HyukjinKwon (via GitHub)" <gi...@apache.org> on 2023/09/25 07:41:08 UTC
[GitHub] [spark] HyukjinKwon opened a new pull request, #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9
HyukjinKwon opened a new pull request, #43098:
URL: https://github.com/apache/spark/pull/43098
### What changes were proposed in this pull request?
This PR removes all conditions related to JDK 9+.
### Why are the changes needed?
- To remove unused code.
- We dropped JDK 8 and 11 at SPARK-44112 so no need to check lower versions conditionally.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
CI in this PR should test them out.
### 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
[GitHub] [spark] HyukjinKwon commented on pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43098:
URL: https://github.com/apache/spark/pull/43098#issuecomment-1733334484
Let me clean them up together here. Thanks!
--
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] HyukjinKwon commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335769270
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:
##########
@@ -115,12 +115,6 @@ private[hive] class HiveClientImpl(
private val outputBuffer = new CircularBuffer()
private val shim = version match {
- case hive.v12 => new Shim_v0_12()
- case hive.v13 => new Shim_v0_13()
- case hive.v14 => new Shim_v0_14()
- case hive.v1_0 => new Shim_v1_0()
- case hive.v1_1 => new Shim_v1_1()
- case hive.v1_2 => new Shim_v1_2()
Review Comment:
Lemme explicitly throw an exception
--
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 #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1336122376
##########
docs/sql-migration-guide.md:
##########
@@ -25,6 +25,7 @@ license: |
## Upgrading from Spark SQL 3.5 to 4.0
- Since Spark 4.0, the default value of `spark.sql.maxSinglePartitionBytes` is changed from `Long.MaxValue` to `128m`. To restore the previous behavior, set `spark.sql.maxSinglePartitionBytes` to `9223372036854775807`(`Long.MaxValue`).
+- Since Spark 4.0, `spark.sql.hive.metastore` drops the support of Hive prior to 2.0.0 as they require JDK 8 that Spark does not support anymore. Users should migrate to higher versions.
Review Comment:
ditto. We need a JIRA.
--
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] HyukjinKwon commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335519470
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientVersions.scala:
##########
@@ -19,16 +19,12 @@ package org.apache.spark.sql.hive.client
import scala.collection.immutable.IndexedSeq
-import org.apache.commons.lang3.{JavaVersion, SystemUtils}
private[client] trait HiveClientVersions {
private val testVersions = sys.env.get("SPARK_TEST_HIVE_CLIENT_VERSIONS")
protected val versions = if (testVersions.nonEmpty) {
testVersions.get.split(",").map(_.trim).filter(_.nonEmpty).toIndexedSeq
- } else if (SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9)) {
- IndexedSeq("2.0", "2.1", "2.2", "2.3", "3.0", "3.1")
} else {
- IndexedSeq("0.12", "0.13", "0.14", "1.0", "1.1", "1.2", "2.0", "2.1", "2.2", "2.3", "3.0",
- "3.1")
+ IndexedSeq("2.0", "2.1", "2.2", "2.3", "3.0", "3.1")
Review Comment:
You're awesome 👍
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientVersions.scala:
##########
@@ -19,16 +19,12 @@ package org.apache.spark.sql.hive.client
import scala.collection.immutable.IndexedSeq
-import org.apache.commons.lang3.{JavaVersion, SystemUtils}
private[client] trait HiveClientVersions {
private val testVersions = sys.env.get("SPARK_TEST_HIVE_CLIENT_VERSIONS")
protected val versions = if (testVersions.nonEmpty) {
testVersions.get.split(",").map(_.trim).filter(_.nonEmpty).toIndexedSeq
- } else if (SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9)) {
- IndexedSeq("2.0", "2.1", "2.2", "2.3", "3.0", "3.1")
} else {
- IndexedSeq("0.12", "0.13", "0.14", "1.0", "1.1", "1.2", "2.0", "2.1", "2.2", "2.3", "3.0",
- "3.1")
+ IndexedSeq("2.0", "2.1", "2.2", "2.3", "3.0", "3.1")
Review Comment:
You're awesome 👍
--
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] HyukjinKwon commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335743366
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala:
##########
@@ -141,8 +140,7 @@ class HiveSparkSubmitSuite
runSparkSubmit(args)
}
- test("SPARK-8020: set sql conf in spark conf") {
- assume(!SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9))
+ ignore("SPARK-8020: set sql conf in spark conf") {
Review Comment:
For this and and the last one, we could. But the others don't set the Hive version apparently. I will just take a look separately.
--
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] LuciferYang commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335578502
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala:
##########
@@ -141,8 +140,7 @@ class HiveSparkSubmitSuite
runSparkSubmit(args)
}
- test("SPARK-8020: set sql conf in spark conf") {
- assume(!SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9))
+ ignore("SPARK-8020: set sql conf in spark conf") {
Review Comment:
Will there be a chance for these ignored cases to be restarted?
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientSuite.scala:
##########
@@ -663,11 +662,10 @@ class HiveClientSuite(version: String, allVersions: Seq[String])
test("sql read hive materialized view") {
// HIVE-14249 Since Hive 2.3.0, materialized view is supported.
- if (version == "2.3" || version == "3.0" || version == "3.1") {
- // Since Hive 3.0(HIVE-19383), we can not run local MR by `client.runSqlHive` with JDK 11.
- assume(version == "2.3" || !SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9))
+ // Since Hive 3.0(HIVE-19383), we can not run local MR by `client.runSqlHive` with JDK 11.
+ if (version == "2.3") {
// Since HIVE-18394(Hive 3.1), "Create Materialized View" should default to rewritable ones
- val disableRewrite = if (version == "2.3" || version == "3.0") "" else "DISABLE REWRITE"
+ val disableRewrite = ""
Review Comment:
still need this `val disableRewrite`?
--
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] HyukjinKwon commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335508134
##########
core/src/main/scala/org/apache/spark/internal/config/UI.scala:
##########
@@ -102,7 +102,7 @@ private[spark] object UI {
val UI_HEAP_HISTOGRAM_ENABLED = ConfigBuilder("spark.ui.heapHistogramEnabled")
.version("3.5.0")
.booleanConf
- .createWithDefault(SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_11))
+ .createWithDefault(true)
Review Comment:
cc @dongjoon-hyun
--
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] HyukjinKwon commented on pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43098:
URL: https://github.com/apache/spark/pull/43098#issuecomment-1733346009
For first and second, they are being fixed in a different PR (as it directly relates to one specific past fix, and I would like the author to 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
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335511325
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientVersions.scala:
##########
@@ -25,10 +25,7 @@ private[client] trait HiveClientVersions {
private val testVersions = sys.env.get("SPARK_TEST_HIVE_CLIENT_VERSIONS")
protected val versions = if (testVersions.nonEmpty) {
testVersions.get.split(",").map(_.trim).filter(_.nonEmpty).toIndexedSeq
- } else if (SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9)) {
- IndexedSeq("2.0", "2.1", "2.2", "2.3", "3.0", "3.1")
} else {
- IndexedSeq("0.12", "0.13", "0.14", "1.0", "1.1", "1.2", "2.0", "2.1", "2.2", "2.3", "3.0",
- "3.1")
+ IndexedSeq("2.0", "2.1", "2.2", "2.3", "3.0", "3.1")
Review Comment:
Yeah. I mean, it doesn't work with the JDK we supported so we should take it as dropping them.
--
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] HyukjinKwon commented on pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43098:
URL: https://github.com/apache/spark/pull/43098#issuecomment-1733093714
cc @dongjoon-hyun
--
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] LuciferYang commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1336592022
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/InsertIntoHiveTable.scala:
##########
@@ -235,28 +235,6 @@ case class InsertIntoHiveTable(
.unwrapped.asInstanceOf[HiveExternalCatalog]
.client
.version
- // SPARK-31684:
Review Comment:
unused import in this file
```
import org.apache.spark.sql.hive.client.hive._
```
--
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] HyukjinKwon commented on pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43098:
URL: https://github.com/apache/spark/pull/43098#issuecomment-1733094588
@LuciferYang and @cloud-fan . They are mostly Hive and SQL direct buffer stuff.
--
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] LuciferYang commented on pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #43098:
URL: https://github.com/apache/spark/pull/43098#issuecomment-1733221595
For the following case, it should also be possible to perform cleanup:
https://github.com/apache/spark/blob/5d422155f1dae09f1631375d09e2f3c8dffba9a5/core/src/test/scala/org/apache/spark/util/UtilsSuite.scala#L1021
https://github.com/apache/spark/blob/5d422155f1dae09f1631375d09e2f3c8dffba9a5/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/TimestampFormatterSuite.scala#L338
--
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] HyukjinKwon commented on pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43098:
URL: https://github.com/apache/spark/pull/43098#issuecomment-1733361945
Let me turn this to draft for now. I think some tests might fail.
--
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] HyukjinKwon commented on pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43098:
URL: https://github.com/apache/spark/pull/43098#issuecomment-1735306269
Oops, I realised that there wasn't an approval here. I can revert or address the comments if there are some. Apologies.
--
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] HyukjinKwon closed pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
URL: https://github.com/apache/spark/pull/43098
--
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] pan3793 commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335508159
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientVersions.scala:
##########
@@ -25,10 +25,7 @@ private[client] trait HiveClientVersions {
private val testVersions = sys.env.get("SPARK_TEST_HIVE_CLIENT_VERSIONS")
protected val versions = if (testVersions.nonEmpty) {
testVersions.get.split(",").map(_.trim).filter(_.nonEmpty).toIndexedSeq
- } else if (SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9)) {
- IndexedSeq("2.0", "2.1", "2.2", "2.3", "3.0", "3.1")
} else {
- IndexedSeq("0.12", "0.13", "0.14", "1.0", "1.1", "1.2", "2.0", "2.1", "2.2", "2.3", "3.0",
- "3.1")
+ IndexedSeq("2.0", "2.1", "2.2", "2.3", "3.0", "3.1")
Review Comment:
Does this mean Spark drops support for HMS below 2.0 officially? Should we drop `HiveShim`s then? e.g. `Shim_v0_12` `Shim_v0_13`
--
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] HyukjinKwon commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335507844
##########
core/src/main/scala/org/apache/spark/internal/config/UI.scala:
##########
@@ -102,7 +102,7 @@ private[spark] object UI {
val UI_HEAP_HISTOGRAM_ENABLED = ConfigBuilder("spark.ui.heapHistogramEnabled")
.version("3.5.0")
.booleanConf
- .createWithDefault(SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_11))
+ .createWithDefault(true)
Review Comment:
I piggyback another change while I am here. I believe these are all instances to fix with `isJavaVersionAtLeast` ... if I didn't miss ..
--
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] LuciferYang commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335580646
##########
core/src/main/scala/org/apache/spark/storage/StorageUtils.scala:
##########
@@ -198,29 +197,14 @@ private[spark] class StorageStatus(
/** Helper methods for storage-related objects. */
private[spark] object StorageUtils extends Logging {
- // In Java 8, the type of DirectBuffer.cleaner() was sun.misc.Cleaner, and it was possible
- // to access the method sun.misc.Cleaner.clean() to invoke it. The type changed to
- // jdk.internal.ref.Cleaner in later JDKs, and the .clean() method is not accessible even with
- // reflection. However sun.misc.Unsafe added a invokeCleaner() method in JDK 9+ and this is
- // still accessible with reflection.
- private val bufferCleaner: DirectBuffer => Unit =
- if (SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9)) {
- val cleanerMethod =
- Utils.classForName("sun.misc.Unsafe").getMethod("invokeCleaner", classOf[ByteBuffer])
- val unsafeField = classOf[Unsafe].getDeclaredField("theUnsafe")
- unsafeField.setAccessible(true)
- val unsafe = unsafeField.get(null).asInstanceOf[Unsafe]
- buffer: DirectBuffer => cleanerMethod.invoke(unsafe, buffer)
- } else {
- val cleanerMethod = Utils.classForName("sun.misc.Cleaner").getMethod("clean")
- buffer: DirectBuffer => {
- // Careful to avoid the return type of .cleaner(), which changes with JDK
- val cleaner: AnyRef = buffer.cleaner()
- if (cleaner != null) {
- cleanerMethod.invoke(cleaner)
- }
- }
- }
+ private val bufferCleaner: DirectBuffer => Unit = {
Review Comment:
For this function, it should be possible to further refactor to avoid using un-exported APIs and internal APIs, but the current changes in this pr are ok.
--
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 a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335737256
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala:
##########
@@ -141,8 +140,7 @@ class HiveSparkSubmitSuite
runSparkSubmit(args)
}
- test("SPARK-8020: set sql conf in spark conf") {
- assume(!SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9))
+ ignore("SPARK-8020: set sql conf in spark conf") {
Review Comment:
shall we change the hive version in these tests?
--
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 #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1336121821
##########
docs/sql-data-sources-hive-tables.md:
##########
@@ -130,7 +130,7 @@ The following options can be used to configure the version of Hive that is used
<td><code>2.3.9</code></td>
<td>
Version of the Hive metastore. Available
- options are <code>0.12.0</code> through <code>2.3.9</code> and <code>3.0.0</code> through <code>3.1.3</code>.
+ options are <code>2.0.0</code> through <code>2.3.9</code> and <code>3.0.0</code> through <code>3.1.3</code>.
Review Comment:
Since this is important, we need a new JIRA for HMS stuff, @HyukjinKwon
--
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] HyukjinKwon commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1336462487
##########
docs/sql-data-sources-hive-tables.md:
##########
@@ -130,7 +130,7 @@ The following options can be used to configure the version of Hive that is used
<td><code>2.3.9</code></td>
<td>
Version of the Hive metastore. Available
- options are <code>0.12.0</code> through <code>2.3.9</code> and <code>3.0.0</code> through <code>3.1.3</code>.
+ options are <code>2.0.0</code> through <code>2.3.9</code> and <code>3.0.0</code> through <code>3.1.3</code>.
Review Comment:
Sure SGTM
--
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] HyukjinKwon commented on pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #43098:
URL: https://github.com/apache/spark/pull/43098#issuecomment-1735305080
Merged to master.
--
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] LuciferYang commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335518191
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientVersions.scala:
##########
@@ -19,16 +19,12 @@ package org.apache.spark.sql.hive.client
import scala.collection.immutable.IndexedSeq
-import org.apache.commons.lang3.{JavaVersion, SystemUtils}
private[client] trait HiveClientVersions {
private val testVersions = sys.env.get("SPARK_TEST_HIVE_CLIENT_VERSIONS")
protected val versions = if (testVersions.nonEmpty) {
testVersions.get.split(",").map(_.trim).filter(_.nonEmpty).toIndexedSeq
- } else if (SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9)) {
- IndexedSeq("2.0", "2.1", "2.2", "2.3", "3.0", "3.1")
} else {
- IndexedSeq("0.12", "0.13", "0.14", "1.0", "1.1", "1.2", "2.0", "2.1", "2.2", "2.3", "3.0",
- "3.1")
+ IndexedSeq("2.0", "2.1", "2.2", "2.3", "3.0", "3.1")
Review Comment:
https://github.com/apache/spark/blob/5d422155f1dae09f1631375d09e2f3c8dffba9a5/sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HivePartitionFilteringSuites.scala#L27
Is this `filterNot` no longer needed as well?
--
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] pan3793 commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335754888
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:
##########
@@ -115,12 +115,6 @@ private[hive] class HiveClientImpl(
private val outputBuffer = new CircularBuffer()
private val shim = version match {
- case hive.v12 => new Shim_v0_12()
- case hive.v13 => new Shim_v0_13()
- case hive.v14 => new Shim_v0_14()
- case hive.v1_0 => new Shim_v1_0()
- case hive.v1_1 => new Shim_v1_1()
- case hive.v1_2 => new Shim_v1_2()
Review Comment:
those code become unreachable now, I'm not sure it's a good idea to retain the dead code.
even though some users may use a modified low-version hive client that supports JDK 17+, changes like https://github.com/apache/spark/pull/42599 would break them silently.
--
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 #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1336120512
##########
core/src/main/scala/org/apache/spark/internal/config/UI.scala:
##########
@@ -102,7 +102,7 @@ private[spark] object UI {
val UI_HEAP_HISTOGRAM_ENABLED = ConfigBuilder("spark.ui.heapHistogramEnabled")
.version("3.5.0")
.booleanConf
- .createWithDefault(SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_11))
+ .createWithDefault(true)
Review Comment:
+1
--
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] LuciferYang commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335505354
##########
connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/ClientE2ETestSuite.scala:
##########
@@ -382,18 +382,16 @@ class ClientE2ETestSuite extends RemoteSparkSession with SQLHelper with PrivateM
test("write jdbc") {
assume(IntegrationTestUtils.isSparkHiveJarAvailable)
- if (SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9)) {
- val url = "jdbc:derby:memory:1234"
- val table = "t1"
- try {
- spark.range(10).write.jdbc(url = s"$url;create=true", table, new Properties())
- val result = spark.read.jdbc(url = url, table, new Properties()).collect()
- assert(result.length == 10)
- } finally {
- // clean up
- assertThrows[SparkException] {
- spark.read.jdbc(url = s"$url;drop=true", table, new Properties()).collect()
- }
+ val url = "jdbc:derby:memory:1234"
Review Comment:
Should also clean up the unused imports, such as `import org.apache.commons.lang3.{JavaVersion, SystemUtils}` in this file
--
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] HyukjinKwon commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335676004
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala:
##########
@@ -115,12 +115,6 @@ private[hive] class HiveClientImpl(
private val outputBuffer = new CircularBuffer()
private val shim = version match {
- case hive.v12 => new Shim_v0_12()
- case hive.v13 => new Shim_v0_13()
- case hive.v14 => new Shim_v0_14()
- case hive.v1_0 => new Shim_v1_0()
- case hive.v1_1 => new Shim_v1_1()
- case hive.v1_2 => new Shim_v1_2()
Review Comment:
I didn't remove `Shim_v0_12` ~ `Shim_v1_2` instances themselves because they inherit each other. I could combine them to one but I think it's fine to leave it as is (as it will show the history at least, and can be useful).
--
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] HyukjinKwon commented on a diff in pull request #43098: [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #43098:
URL: https://github.com/apache/spark/pull/43098#discussion_r1335673739
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala:
##########
@@ -141,8 +140,7 @@ class HiveSparkSubmitSuite
runSparkSubmit(args)
}
- test("SPARK-8020: set sql conf in spark conf") {
- assume(!SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_9))
+ ignore("SPARK-8020: set sql conf in spark conf") {
Review Comment:
Let's leave it for now. I think this test does not target to test 0.12 (although the metastore version is set like that).
--
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