[SPARK-37091][R] SystemRequirements to include Java < 18 #34371
Conversation
|
Can one of the admins verify this patch? |
dongjoon-hyun
changed the title
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Thank you for the suggestion, @Bidek56 . Could you include your actual result if you test SparkR with Java 17 already?
$ build/mvn -Phadoop-3.2 -Psparkr -DskipTests package
$ R/install-dev.sh
$ R/run-tests.sh
At least, I agree that this PR unblocks SparkR installation on Java 17.
R/pkg/DESCRIPTION
Outdated
| URL: https://www.apache.org https://spark.apache.org | ||
| BugReports: https://spark.apache.org/contributing.html | ||
| SystemRequirements: Java (>= 8, < 12) | ||
| SystemRequirements: Java (>= 8, <= 17) |
There was a problem hiding this comment.
Hey, I would like to double check this. Why does this matter? What issue did you face by this?
|
I agree with this change, and LGTM but I would like to understand how this change interacts with something external. |
This issue is affected by the Java 11 limit |
|
https://github.com/jupyter/drumsticks repo seems private. Okay, if something is affected, then it's fine - I asked because this Java versions in description are not used in CRAN and rather just a metadata up to my knowledge. |
|
Mind enabling GitHub Actions in your forked repository? Apache Spark leverages contributor's resources in forked repository in their PRs (https://github.com/apache/spark/pull/34371/checks?check_run_id=3977747161). |
It's a public repo |
|
https://github.com/jupyter/drumsticks: I cannot access 😢 . Once it's enabled, you could rebase in this PR. That should kick the job |
### What changes were proposed in this pull request? Set `spark.sql.files.openCostInBytes` to bytesConf. ### Why are the changes needed? The name is _*InBytes_, but it actually only accepts **long type**. This is confusing for users. After the changes, it can accept **String** as input which is more flexible to users. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing test. Closes #34353 from RabbidHY/SPARK-37084. Authored-by: RabbidHY <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
|
I also can't see the repo, but w/e. It seems fine to make this change - not sure any tests will test it anyway. |
|
CRAN check in SparkR build validates the DESCRIPTION file. The check won't validate the values but at least format and etc. Might need to make sure that the tests pass anyway although it's unlikely that the tests are broken by this change. |
It's a public repo, not sure where the drumsticks came from, maybe Android share link, sorry. |
Bidek56
changed the title
core/src/test/scala/org/apache/spark/internal/config/ConfigEntrySuite.scala
Outdated
Show resolved
Hide resolved
…esConf" This reverts commit 3b3c426.
|
This looks fine now. It wasn't a test problem, just some incorrect commits got merged when you rebased this branch. All set |
|
Actually, I think the current PR is mergable and should look fine since it will be squashed .. although it should ideally be rebased. Merged to master. |
|
@HyukjinKwon , the final commit looks still wrong to me. Could you check the final commit before pushing to the Apache repo, @HyukjinKwon ? |
|
oops |
|
This is a bit odds because the PR shows the diff only the one in description https://github.com/apache/spark/pull/34371/files, and I haven't had any problem in such cases so far. |
|
sorry it was my bad. let me correct this one. |
|
Ya, this commit technically reverted |
|
No problem, @HyukjinKwon ~ We are okay because this is not released and we can fix it. |
|
I cherry-picked 76a317a back to the master branch. |
6 participants
What changes were proposed in this pull request?
Modifying SystemRequirements to include Java <= 17
Why are the changes needed?
Currently Java is limited to version 11
Does this PR introduce any user-facing change? No
How was this patch tested?