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

[WIP][SPARK-35253][BUILD][SQL] Upgrade Janino from 3.0.16 to 3.1.3 #32374

Closed
wants to merge 3 commits into from

Conversation

LuciferYang
Copy link
Contributor

What changes were proposed in this pull request?

This PR proposes to upgrade Janino from 3.0.16 to 3.1.3 because the 3.0.x line is deprecated (janino-change-log).

Why are the changes needed?

the Janino 3.0.x line is deprecated

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass the Jenkins or GitHub Action

@LuciferYang LuciferYang changed the title [SPARK-35253][BUILD][SQL] Upgrade Janino from 3.0.16 to 3.1.3 [WIP][SPARK-35253][BUILD][SQL] Upgrade Janino from 3.0.16 to 3.1.3 Apr 28, 2021
@SparkQA
Copy link

SparkQA commented Apr 28, 2021

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42546/

@LuciferYang LuciferYang marked this pull request as draft April 28, 2021 07:26
@SparkQA
Copy link

SparkQA commented Apr 28, 2021

Test build #138027 has finished for PR 32374 at commit 3674ee1.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Apr 28, 2021

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42550/

@SparkQA
Copy link

SparkQA commented Apr 28, 2021

Test build #138031 has finished for PR 32374 at commit a2b7d11.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Apr 28, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42564/

@SparkQA
Copy link

SparkQA commented Apr 28, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42564/

@SparkQA
Copy link

SparkQA commented Apr 28, 2021

Test build #138045 has finished for PR 32374 at commit 25daf52.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@LuciferYang
Copy link
Contributor Author

I found SPARK-31214 (#27860) was reversed, I'm not sure if we need to upgrade this lib, there are still some UT failures now.

@LuciferYang
Copy link
Contributor Author

It seems that SPARK-32640 has not been solved with Janino 3.1.3

@maropu
Copy link
Member

maropu commented Apr 30, 2021

Hi, @LuciferYang. Since janino-3.1.3 has some critical issues, I think spark cannot land on it. So, I'm fixing janino these issues, e.g.,

@LuciferYang
Copy link
Contributor Author

@maropu Thank you for your feedback, maybe we need to wait until the next version of Janino to upgrade it

@LuciferYang
Copy link
Contributor Author

@maropu After janino-compiler/janino#145 and janino-compiler/janino#146 merged, I will try to use the snapshot version to verify whether there are other blocking issue

@LuciferYang
Copy link
Contributor Author

close this because SPARK-35253

srowen pushed a commit that referenced this pull request May 12, 2021
### What changes were proposed in this pull request?

This PR proposes to bump up the janino version from 3.0.16 to v3.1.4.
The major changes of this upgrade are as follows:
 - Fixed issue #131: Janino 3.1.2 is 10x slower than 3.0.11: The Compiler's IClassLoader was initialized way too eagerly, thus lots of classes were loaded from the class path, which is very slow.
 - Improved the encoding of stack map frames according to JVMS11 4.7.4: Previously, only "full_frame"s were generated.
 - Fixed issue #107: Janino requires "org.codehaus.commons.compiler.io", but commons-compiler does not export this package
 - Fixed the promotion of the array access index expression (see JLS7 15.13 Array Access Expressions).

For all the changes, please see the change log: http://janino-compiler.github.io/janino/changelog.html

NOTE1: I've checked that there is no obvious performance regression. For all the data, see a link: https://docs.google.com/spreadsheets/d/1srxT9CioGQg1fLKM3Uo8z1sTzgCsMj4pg6JzpdcG6VU/edit?usp=sharing

NOTE2: We upgraded janino to 3.1.2 (#27860) once before, but the commit had been reverted in #29495 because of the correctness issue. Recently, #32374 had checked if Spark could land on v3.1.3 or not, but a new bug was found there. These known issues has been fixed in v3.1.4 by following PRs:
 - janino-compiler/janino#145
 - janino-compiler/janino#146

### Why are the changes needed?

janino v3.0.X  is no longer maintained.

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

No.

### How was this patch tested?

GA passed.

Closes #32455 from maropu/janino_v3.1.4.

Authored-by: Takeshi Yamamuro <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
@LuciferYang LuciferYang deleted the SPARK-35253 branch June 6, 2022 03:47
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.

3 participants