Skip to content

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Aug 24, 2023

What changes were proposed in this pull request?

This PR aims to re-enable Arrow-based connect tests in Java 21.
This depends on #42181.

Why are the changes needed?

To have Java 21 test coverage.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

$ java -version
openjdk version "21-ea" 2023-09-19
OpenJDK Runtime Environment (build 21-ea+32-2482)
OpenJDK 64-Bit Server VM (build 21-ea+32-2482, mixed mode, sharing)

$ build/sbt "connect/test" -Phive
...
[info] Run completed in 14 seconds, 136 milliseconds.
[info] Total number of tests run: 858
[info] Suites: completed 20, aborted 0
[info] Tests: succeeded 858, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 44 s, completed Aug 23, 2023, 9:42:53 PM

$ build/sbt "connect-client-jvm/test" -Phive
...
[info] Run completed in 1 minute, 24 seconds.
[info] Total number of tests run: 1220
[info] Suites: completed 24, aborted 0
[info] Tests: succeeded 1220, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[info] Passed: Total 1222, Failed 0, Errors 0, Passed 1222

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

No.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-44121][SQL][TESTS] Renable Arrow-based connect tests in Java 21 [SPARK-44121][CONNECT][TESTS] Renable Arrow-based connect tests in Java 21 Aug 24, 2023
* SPARK-44259: override test function to skip `RemoteSparkSession-based` tests as default, we
* should delete this function after SPARK-44121 is completed.
*/
override protected def test(testName: String, testTags: Tag*)(testFun: => Any)(implicit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, please ignore my previous comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem at all. Thank you for review!

}

override def beforeAll(): Unit = {
// TODO(SPARK-44121) Remove this check condition
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest these changes could be merged into 3.5 to avoid future conflicts. WDYT @dongjoon-hyun

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least the current one related to connect.

Copy link
Member Author

@dongjoon-hyun dongjoon-hyun Aug 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this connect area, I'll follow your guideline because you are the original author, @LuciferYang . :)

@dongjoon-hyun dongjoon-hyun marked this pull request as ready for review August 24, 2023 06:36
@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Aug 24, 2023

Could you review this PR, @LuciferYang ? The connect module CI passed and I verified this test case PR locally on Java 21.

Copy link
Contributor

@LuciferYang LuciferYang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also manually verified it with Java 21.
LGTM ~

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Aug 24, 2023

Thank you so much, @LuciferYang !
Merged to master/3.5.

dongjoon-hyun added a commit that referenced this pull request Aug 24, 2023
…va 21

### What changes were proposed in this pull request?

This PR aims to re-enable Arrow-based connect tests in Java 21.
This depends on #42181.

### Why are the changes needed?

To have Java 21 test coverage.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

```
$ java -version
openjdk version "21-ea" 2023-09-19
OpenJDK Runtime Environment (build 21-ea+32-2482)
OpenJDK 64-Bit Server VM (build 21-ea+32-2482, mixed mode, sharing)

$ build/sbt "connect/test" -Phive
...
[info] Run completed in 14 seconds, 136 milliseconds.
[info] Total number of tests run: 858
[info] Suites: completed 20, aborted 0
[info] Tests: succeeded 858, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 44 s, completed Aug 23, 2023, 9:42:53 PM

$ build/sbt "connect-client-jvm/test" -Phive
...
[info] Run completed in 1 minute, 24 seconds.
[info] Total number of tests run: 1220
[info] Suites: completed 24, aborted 0
[info] Tests: succeeded 1220, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[info] Passed: Total 1222, Failed 0, Errors 0, Passed 1222
```

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

No.

Closes #42643 from dongjoon-hyun/SPARK-44121.

Authored-by: Dongjoon Hyun <dhyun@apple.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
(cherry picked from commit a824a6d)
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
@dongjoon-hyun
Copy link
Member Author

To all, although branch-3.5 doesn't support Java 21, this recover commit landed at branch-3.5 to reduce the diff between master and branch-3.5 branches as discussed here (#42643 (comment)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants