Skip to content
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

Use scala Int.box instead of Integer constructors #4338

Merged
merged 4 commits into from
Dec 10, 2021

Conversation

abellina
Copy link
Collaborator

@abellina abellina commented Dec 9, 2021

This is a small fix for the test code where it was using new Integer instead of Int.box. The integer constructors were deprecated in JDK 11, so this is not a must have for us, but it is not a great user experience to have the plugin fail to build because of this.

@sameerz up to you if I should target this to 21.12. @jlowe @revans2 fyi.

@abellina abellina self-assigned this Dec 9, 2021
@abellina abellina added the bug Something isn't working label Dec 9, 2021
…n JDK11

Signed-off-by: Alessandro Bellina <abellina@nvidia.com>
mythrocks
mythrocks previously approved these changes Dec 9, 2021
Copy link
Collaborator

@mythrocks mythrocks left a comment

Choose a reason for hiding this comment

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

Wow. TIL.

LGTM! lolkthxby.

@jlowe jlowe added the test Only impacts tests label Dec 9, 2021
jlowe
jlowe previously approved these changes Dec 9, 2021
@abellina
Copy link
Collaborator Author

abellina commented Dec 9, 2021

build

@sameerz
Copy link
Collaborator

sameerz commented Dec 9, 2021

@sameerz up to you if I should target this to 21.12. @jlowe @revans2 fyi.

We do not support Java 11 in 21.12, so let's target this for the next release.

gerashegalov
gerashegalov previously approved these changes Dec 9, 2021
Copy link
Collaborator

@gerashegalov gerashegalov left a comment

Choose a reason for hiding this comment

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

LGTM, nit

@@ -165,7 +165,7 @@ object RapidsShuffleTestHelper extends MockitoSugar with Arm {
}

def withMockContiguousTable[T](numRows: Long)(body: ContiguousTable => T): T = {
val rows: Seq[Integer] = (0 until numRows.toInt).map(new Integer(_))
val rows: Seq[Integer] = (0 until numRows.toInt).map(Int.box)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change
val rows: Seq[Integer] = (0 until numRows.toInt).map(Int.box)
val rows: Seq[Integer] = (0 until numRows).map(Int.box)

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry, that was a bad comment, scalac won't do a silent lossy conversion for us. I'll undo it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gerashegalov no problem, the constructor (I was trying not to use) also came back with the revert, but I changed it back to Int.box. Please take another look!

Co-authored-by: Gera Shegalov <gshegalov@nvidia.com>
@abellina abellina dismissed stale reviews from gerashegalov, jlowe, and mythrocks via 2015c2f December 9, 2021 21:50
@abellina
Copy link
Collaborator Author

abellina commented Dec 9, 2021

build

Signed-off-by: Gera Shegalov <gera@apache.org>
@abellina
Copy link
Collaborator Author

build

@abellina abellina merged commit 024e5cb into NVIDIA:branch-22.02 Dec 10, 2021
@abellina abellina deleted the bug/use_scala_box branch December 10, 2021 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working test Only impacts tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants