[SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17 #43098

HyukjinKwon · 2023-09-25T07:41:06Z

What changes were proposed in this pull request?

This PR removes all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17.

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.

HyukjinKwon · 2023-09-25T07:41:14Z

cc @dongjoon-hyun

HyukjinKwon · 2023-09-25T07:41:49Z

@LuciferYang and @cloud-fan . They are mostly Hive and SQL direct buffer stuff.

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/ClientE2ETestSuite.scala

core/src/main/scala/org/apache/spark/internal/config/UI.scala

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientVersions.scala

LuciferYang · 2023-09-25T08:32:54Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientSuite.scala

still need this val disableRewrite?

LuciferYang · 2023-09-25T08:46:20Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/HiveSparkSubmitSuite.scala

Will there be a chance for these ignored cases to be restarted?

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).

shall we change the hive version in these tests?

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.

LuciferYang · 2023-09-25T08:48:11Z

core/src/main/scala/org/apache/spark/storage/StorageUtils.scala

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.

LuciferYang · 2023-09-25T09:02:48Z

For the following case, it should also be possible to perform cleanup:

spark/core/src/test/scala/org/apache/spark/util/UtilsSuite.scala

Line 1021 in 5d42215

if (SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_1_8)) {

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/TimestampFormatterSuite.scala

Line 338 in 5d42215

if (SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_13)) {

Additionally, the SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_17) in the following case should also be able to be cleaned up:

spark/core/src/main/scala/org/apache/spark/util/ClosureCleaner.scala

Line 424 in 5d42215

if (SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_17) &&

spark/connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/SQLImplicitsTestSuite.scala

Line 155 in 5d42215

    
           if (SystemUtils.isJavaVersionAtLeast(JavaVersion.JAVA_17) && SystemUtils.IS_OS_LINUX) {

Of course, they can also belong to a separate pr

HyukjinKwon · 2023-09-25T09:51:45Z

Let me clean them up together here. Thanks!

HyukjinKwon · 2023-09-25T09:59:15Z

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).

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

HyukjinKwon · 2023-09-25T10:09:07Z

Let me turn this to draft for now. I think some tests might fail.

docs/sql-data-sources-hive-tables.md

docs/sql-migration-guide.md

dongjoon-hyun

Mostly this PR is right, but we had better drop old HMS independently first.

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/InsertIntoHiveTable.scala

HyukjinKwon · 2023-09-26T10:58:55Z

Merged to master.

HyukjinKwon · 2023-09-26T10:59:47Z

Oops, I realised that there wasn't an approval here. I can revert or address the comments if there are some. Apologies.

…eTable` ### What changes were proposed in this pull request? This pr aims to cleans up the unused local variables `partitionPath`, `hiveVersion`, and `doHiveOverwrite` in `InsertIntoHiveTable`. The code that used these variables has already been cleaned up in SPARK-45309 | #43098. ### Why are the changes needed? Code cleanup. ### 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 #44433 from LuciferYang/insert-into-hive-table. Lead-authored-by: yangjie01 <yangjie01@baidu.com> Co-authored-by: YangJie <yangjie01@baidu.com> Signed-off-by: Kent Yao <yao@apache.org>

github-actions bot added SQL CORE CONNECT labels Sep 25, 2023

LuciferYang reviewed Sep 25, 2023

View reviewed changes

connector/connect/client/jvm/src/test/scala/org/apache/spark/sql/ClientE2ETestSuite.scala Outdated Show resolved Hide resolved

github-actions bot added the WEB UI label Sep 25, 2023

HyukjinKwon commented Sep 25, 2023

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/UI.scala Outdated Show resolved Hide resolved

pan3793 reviewed Sep 25, 2023

View reviewed changes

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientVersions.scala Outdated Show resolved Hide resolved

LuciferYang reviewed Sep 25, 2023

View reviewed changes

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HiveClientVersions.scala Outdated Show resolved Hide resolved

HyukjinKwon changed the title ~~[SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9~~ [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11 Sep 25, 2023

LuciferYang reviewed Sep 25, 2023

View reviewed changes

HyukjinKwon changed the title ~~[SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11~~ [SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17 Sep 25, 2023

HyukjinKwon commented Sep 25, 2023

View reviewed changes

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala Outdated Show resolved Hide resolved

HyukjinKwon marked this pull request as draft September 25, 2023 10:08

github-actions bot added the DOCS label Sep 25, 2023

HyukjinKwon force-pushed the SPARK-45309 branch 2 times, most recently from 7e09f07 to f9296bb Compare September 25, 2023 13:16

dongjoon-hyun reviewed Sep 25, 2023

View reviewed changes

docs/sql-data-sources-hive-tables.md Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Sep 25, 2023

View reviewed changes

docs/sql-migration-guide.md Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Sep 25, 2023

View reviewed changes

HyukjinKwon force-pushed the SPARK-45309 branch from f9296bb to 874fe55 Compare September 26, 2023 03:22

github-actions bot removed the DOCS label Sep 26, 2023

HyukjinKwon marked this pull request as ready for review September 26, 2023 03:23

Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17

8493474

HyukjinKwon force-pushed the SPARK-45309 branch from 874fe55 to 8493474 Compare September 26, 2023 03:25

LuciferYang reviewed Sep 26, 2023

View reviewed changes

sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/InsertIntoHiveTable.scala Show resolved Hide resolved

HyukjinKwon added 2 commits September 26, 2023 12:58

Unused import

cea7735

nit

d0eab33

HyukjinKwon closed this in e54c866 Sep 26, 2023

LuciferYang mentioned this pull request Dec 21, 2023

[SPARK-46469][SQL] Clean up useless local variables in InsertIntoHiveTable #44433

Closed

HyukjinKwon deleted the SPARK-45309 branch January 15, 2024 00:51

[SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17 #43098

[SPARK-45309][SQL] Remove all SystemUtils.isJavaVersionAtLeast with JDK 9/11/17 #43098

Uh oh!

Conversation

HyukjinKwon commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

HyukjinKwon commented Sep 25, 2023

Uh oh!

HyukjinKwon commented Sep 25, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LuciferYang Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

LuciferYang Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

cloud-fan Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

LuciferYang Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Sep 25, 2023

Uh oh!

HyukjinKwon commented Sep 25, 2023

Uh oh!

Uh oh!

HyukjinKwon commented Sep 25, 2023

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HyukjinKwon commented Sep 26, 2023

Uh oh!

HyukjinKwon commented Sep 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HyukjinKwon commented Sep 25, 2023 •

edited

Loading

LuciferYang commented Sep 25, 2023 •

edited

Loading