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

[SPARK-22688][SQL] Upgrade Janino version to 3.0.8 #19890

Closed
wants to merge 2 commits into from

Conversation

@kiszk
Copy link
Member

commented Dec 5, 2017

What changes were proposed in this pull request?

This PR upgrade Janino version to 3.0.8. Janino 3.0.8 includes an important fix to reduce the number of constant pool entries by using 'sipush' java bytecode.

  • SIPUSH bytecode is not used for short integer constant #33.

Please see detail in this discussion thread.

How was this patch tested?

Existing tests

@srowen
srowen approved these changes Dec 5, 2017
Copy link
Member

left a comment

Sounds good. This should also go back to branch-2.1 like the previous update to 3.0.7, but note that there are more deps files to update in that branch.

@SparkQA

This comment has been minimized.

Copy link

commented Dec 5, 2017

Test build #84465 has finished for PR 19890 at commit 68b24c9.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
@kiszk

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2017

@srowen thank you for pointing it out.
Here is an example at 3.0.7.

@gatorsmile

This comment has been minimized.

Copy link
Member

commented Dec 5, 2017

Wait...

Renamed the "JaninoRuntimeException" to "InternalCompilerException" (and left the old class for backwards compatibility), because the old name was confusing.

We captured JaninoRuntimeException. Does that mean we need to also Capture InternalCompilerException if we upgrade it?

@kiszk Could you double check it?

@kiszk

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2017

@gatorsmile great catch, I will double check it.

@kiszk

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2017

@gatorsmile The current code works functionally correct since InternalCompilerException is defined as a subclass of JaninoRuntimeException. On the other hand, JaninoRuntimeException is deprecated now.

Do we replace JaninoRuntimeException in Spark code with InternalCompilerException?
I do not have strong preference.

@HyukjinKwon

This comment has been minimized.

Copy link
Member

commented Dec 6, 2017

Yea, let's avoid using deprecated ones, of course if it does not break anything.

@kiszk

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2017

Sure

@SparkQA

This comment has been minimized.

Copy link

commented Dec 6, 2017

Test build #84545 has finished for PR 19890 at commit 663803b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
@srowen

This comment has been minimized.

Copy link
Member

commented Dec 6, 2017

It would only cause a problem if somehow and older Janino was on the classpath, which does not have the new exception type. I don't think there's a reason to believe that happens in practice (i.e. not brought in by Hadoop).

@gatorsmile

This comment has been minimized.

Copy link
Member

commented Dec 7, 2017

@rednaxelafx

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2017

I'd LGTM on both upgrading to Janino 3.0.8 and also changing the exception type captured from JaninoRuntimeException to the new InternalCompilerException. It's never a good idea to have conflicting library version on the classpath anyway, hitting an error in terms of ClassNotFoundException or NoClassDefFoundError on this new exception type is actually a good hint that there is a version conflict -- it'd be nice if we can document this knowledge somewhere easy to find, though.

Copy link
Member

left a comment

LGTM

@gatorsmile

This comment has been minimized.

Copy link
Member

commented Dec 7, 2017

Thanks! Merged to master.

@asfgit asfgit closed this in 8ae004b Dec 7, 2017
@srowen

This comment has been minimized.

Copy link
Member

commented Dec 7, 2017

We'll need to back port to 2.2/2.1 to match the previous janino update. I'll do that. A different PR will be required for 2.1

asfgit pushed a commit that referenced this pull request Dec 7, 2017
This PR upgrade Janino version to 3.0.8. [Janino 3.0.8](https://janino-compiler.github.io/janino/changelog.html) includes an important fix to reduce the number of constant pool entries by using 'sipush' java bytecode.

* SIPUSH bytecode is not used for short integer constant [#33](janino-compiler/janino#33).

Please see detail in [this discussion thread](#19518 (comment)).

Existing tests

Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com>

Closes #19890 from kiszk/SPARK-22688.

(cherry picked from commit 8ae004b)
Signed-off-by: Sean Owen <sowen@cloudera.com>
asfgit pushed a commit that referenced this pull request Dec 7, 2017
## What changes were proposed in this pull request?

Hotfix inadvertent change to xmlbuilder dep when updating Janino.
See backport of #19890

## How was this patch tested?

N/A

Author: Sean Owen <sowen@cloudera.com>

Closes #19922 from srowen/SPARK-22688.2.
asfgit pushed a commit that referenced this pull request Dec 8, 2017
This PR upgrade Janino version to 3.0.8. [Janino 3.0.8](https://janino-compiler.github.io/janino/changelog.html) includes an important fix to reduce the number of constant pool entries by using 'sipush' java bytecode.

* SIPUSH bytecode is not used for short integer constant [#33](janino-compiler/janino#33).

Please see detail in [this discussion thread](#19518 (comment)).

Existing tests

Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com>

Closes #19890 from kiszk/SPARK-22688.

(cherry picked from commit 8ae004b)
Signed-off-by: Sean Owen <sowen@cloudera.com>
MatthewRBruce added a commit to Shopify/spark that referenced this pull request Jul 31, 2018
This PR upgrade Janino version to 3.0.8. [Janino 3.0.8](https://janino-compiler.github.io/janino/changelog.html) includes an important fix to reduce the number of constant pool entries by using 'sipush' java bytecode.

* SIPUSH bytecode is not used for short integer constant [apache#33](janino-compiler/janino#33).

Please see detail in [this discussion thread](apache#19518 (comment)).

Existing tests

Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com>

Closes apache#19890 from kiszk/SPARK-22688.

(cherry picked from commit 8ae004b)
Signed-off-by: Sean Owen <sowen@cloudera.com>
MatthewRBruce added a commit to Shopify/spark that referenced this pull request Jul 31, 2018
## What changes were proposed in this pull request?

Hotfix inadvertent change to xmlbuilder dep when updating Janino.
See backport of apache#19890

## How was this patch tested?

N/A

Author: Sean Owen <sowen@cloudera.com>

Closes apache#19922 from srowen/SPARK-22688.2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.