[SPARK-35253][SQL][BUILD] Bump up the janino version to v3.1.4 by maropu · Pull Request #32455 · apache/spark

maropu · 2021-05-06T14:07:13Z

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 [SPARK-1237, 1238] Improve the computation of YtY for implicit ALS #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 SPARK-1190: Do not initialize log4j if slf4j log4j backend is not being used #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:

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.

maropu · 2021-05-06T14:11:31Z

I've fixed some issues in janino and the latest janino (w/ these fixes) has been released hours ago.

I will check if the existing tests can pass and there is no TPCDS performance regression.

SparkQA · 2021-05-06T14:58:21Z

Kubernetes integration test unable to build dist.

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

dongjoon-hyun · 2021-05-06T15:25:18Z

Thank you, @maropu . Yes, it will be great if we can succeed to upgrade at this time.

kiszk · 2021-05-06T16:07:05Z

It would be great if we could check some compilation time.
Janino may have degradation regarding the compilation time like #29495 (comment)

SparkQA · 2021-05-06T16:36:40Z

Test build #138213 has finished for PR 32455 at commit 2742916.

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

viirya · 2021-05-06T21:42:06Z

Looks like all tests are passed!

SparkQA · 2021-05-07T00:45:45Z

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

SparkQA · 2021-05-07T00:45:46Z

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

SparkQA · 2021-05-07T02:22:38Z

Test build #138219 has finished for PR 32455 at commit 8a13cfb.

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

HyukjinKwon · 2021-05-07T09:44:30Z

cc @rednaxelafx too FYI

maropu · 2021-05-07T12:36:24Z

okay, as far as I checked, I couldn't find any critical problem. I think it is good timing to re-start the discussion to decide whether we upgrade janino or not for the next release (v3.2.0). WDYT, guys? @gatorsmile @cloud-fan @HyukjinKwon @dongjoon-hyun @viirya @kiszk @rednaxelafx @HeartSaVioR @gengliangwang @tgravescs @LuciferYang (I've listed up the members who participated in the previous discussions)

tgravescs · 2021-05-07T13:12:33Z

3.2.0 sounds fine to me. From the description it seems like the issue in https://issues.apache.org/jira/browse/SPARK-32640 was fixed with janino-compiler/janino#145 so I'm good with it. It might be nice to add a test like SPARK-32640 if we don't have one yet.

viirya · 2021-05-07T20:37:18Z

There was a test added in SPARK-32640. So looks it is fixed now as all tests are passed. If no regression found, looks okay.

dongjoon-hyun

+1, LGTM from my side.

rednaxelafx · 2021-05-07T23:13:37Z

I'll do some experiments in my environment as well. Thank you very much for all the efforts driving the Janino upgrade, @maropu san!

gengliangwang

+1. I just tried it locally and the bug described in https://issues.apache.org/jira/browse/SPARK-32640 is fixed.
Thanks for the work.

srowen · 2021-05-09T13:22:25Z

Yes I'd update to get all those fixes if there's no reason to expect a problem now.

LuciferYang · 2021-05-10T02:30:54Z

-      val resultField = classOf[SimpleCompiler].getDeclaredField("result")
-      resultField.setAccessible(true)
-      val loader = resultField.get(evaluator).asInstanceOf[ByteArrayClassLoader]
+      val scField = classOf[ClassBodyEvaluator].getDeclaredField("sc")


@maropu Can we directly use evaluator.getBytecodes.asScala instead of line 1438 ~ line 1445?

Is the data the totally same with ByteArrayClassLoader.classes? The suggestion is a trivial update though, IMO this PR just focuses on the janino upgrade. Could you make a follow-up PR for it after this merged?

@maropu ok, I can try to do this followup :)

srowen · 2021-05-12T13:58:28Z

Merged to master

maropu · 2021-05-13T00:53:40Z

Thank you, all the reviewers~

maropu · 2021-07-12T06:56:08Z

@cloud-fan @HyukjinKwon As you know, the janino community is pretty inactive now. So, I'm wondering if it would be better to revert this commit from branch-3.2 before the QA period. WDYT? cc: @dongjoon-hyun @viirya

LuciferYang · 2021-07-12T07:31:26Z

@maropu If we need to revert this commit, remember to revert SPARK-35398 first

srowen · 2021-07-12T11:48:06Z

What's the need to revert it?

cloud-fan · 2021-07-12T14:07:49Z

Because the new janino version has bugs that can fail certain SQL queries, which is a regression.

srowen · 2021-07-12T14:12:34Z

Sounds good, revert it

cloud-fan · 2021-07-12T14:27:52Z

I'll open a revert PR soon. If janino can have a new release with the bug fixed before rc1, we can revert my revert.

dongjoon-hyun · 2021-07-12T14:31:02Z

+1 for reverting, too.

gengliangwang · 2021-07-12T14:33:02Z

+1 for reverting. Thank you @maropu @cloud-fan

viirya · 2021-07-12T16:05:32Z

+1 for reverting it. Thanks!

cloud-fan · 2021-07-12T16:50:15Z

The PR is out: #33302

…sion to v3.1.4" ### What changes were proposed in this pull request? This PR reverts #32455 and its followup #32536 , because the new janino version has a bug that is not fixed yet: janino-compiler/janino#148 ### Why are the changes needed? avoid regressions ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests Closes #33302 from cloud-fan/revert. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

…sion to v3.1.4" ### What changes were proposed in this pull request? This PR reverts #32455 and its followup #32536 , because the new janino version has a bug that is not fixed yet: janino-compiler/janino#148 ### Why are the changes needed? avoid regressions ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? existing tests Closes #33302 from cloud-fan/revert. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit ae6199a) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

github-actions Bot added BUILD SQL labels May 6, 2021

maropu marked this pull request as draft May 6, 2021 14:07

maropu changed the title ~~[WIP][SPARK-XXXXX][SQL][BUILD] Bump up the janino version to v3.1.4(latest)~~ [WIP][SPARK-35253][SQL][BUILD] Bump up the janino version to v3.1.4(latest) May 6, 2021

maropu added 2 commits May 7, 2021 09:00

Bump up janino version to 3.1.3

d1d996b

Fix

8a13cfb

maropu force-pushed the janino_v3.1.4 branch from 2742916 to 8a13cfb Compare May 7, 2021 00:01

maropu changed the title ~~[WIP][SPARK-35253][SQL][BUILD] Bump up the janino version to v3.1.4(latest)~~ [SPARK-35253][SQL][BUILD] Bump up the janino version to v3.1.4(latest) May 7, 2021

maropu marked this pull request as ready for review May 7, 2021 12:29

dongjoon-hyun changed the title ~~[SPARK-35253][SQL][BUILD] Bump up the janino version to v3.1.4(latest)~~ [SPARK-35253][SQL][BUILD] Bump up the janino version to v3.1.4 May 7, 2021

dongjoon-hyun approved these changes May 7, 2021

View reviewed changes

gengliangwang approved these changes May 8, 2021

View reviewed changes

viirya approved these changes May 8, 2021

View reviewed changes

HyukjinKwon approved these changes May 8, 2021

View reviewed changes

LuciferYang reviewed May 10, 2021

View reviewed changes

LuciferYang approved these changes May 11, 2021

View reviewed changes

cloud-fan approved these changes May 11, 2021

View reviewed changes

srowen closed this in 101b0cc May 12, 2021

maropu mentioned this pull request May 13, 2021

Fix the bug that handles NaN comparisons incorrectly janino-compiler/janino#145

Merged

cloud-fan mentioned this pull request Jul 12, 2021

Revert "[SPARK-35253][SPARK-35398][SQL][BUILD] Bump up the janino version to v3.1.4" #33302

Closed

Uh oh!

Conversation

maropu commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

maropu commented May 6, 2021

Uh oh!

SparkQA commented May 6, 2021

Uh oh!

dongjoon-hyun commented May 6, 2021

Uh oh!

kiszk commented May 6, 2021

Uh oh!

SparkQA commented May 6, 2021

Uh oh!

viirya commented May 6, 2021

Uh oh!

SparkQA commented May 7, 2021

Uh oh!

SparkQA commented May 7, 2021

Uh oh!

SparkQA commented May 7, 2021

Uh oh!

HyukjinKwon commented May 7, 2021

Uh oh!

maropu commented May 7, 2021

Uh oh!

tgravescs commented May 7, 2021

Uh oh!

viirya commented May 7, 2021

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

rednaxelafx commented May 7, 2021

Uh oh!

gengliangwang left a comment

Choose a reason for hiding this comment

Uh oh!

srowen commented May 9, 2021

Uh oh!

LuciferYang May 10, 2021

Choose a reason for hiding this comment

Uh oh!

maropu May 10, 2021

Choose a reason for hiding this comment

Uh oh!

LuciferYang May 11, 2021

Choose a reason for hiding this comment

Uh oh!

srowen commented May 12, 2021

Uh oh!

maropu commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maropu commented Jul 12, 2021

Uh oh!

LuciferYang commented Jul 12, 2021

Uh oh!

srowen commented Jul 12, 2021

Uh oh!

cloud-fan commented Jul 12, 2021

Uh oh!

srowen commented Jul 12, 2021

Uh oh!

cloud-fan commented Jul 12, 2021

Uh oh!

dongjoon-hyun commented Jul 12, 2021

Uh oh!

gengliangwang commented Jul 12, 2021

Uh oh!

viirya commented Jul 12, 2021

Uh oh!

cloud-fan commented Jul 12, 2021

Uh oh!

Reviewers

maropu commented May 6, 2021 •

edited

Loading

maropu commented May 13, 2021 •

edited

Loading