Padding for TABLESWITCH can be applied multiple times due to grow on relocatables #113
Comments
|
Actually I've crafted a patch but I'm not expert of JVM (just dug up with JLS only on TABLESWITCH) so not sure I assumed correctly. My assumptions are:
If my assumptions are all correct, fixUpAndRelocate can just execute three phases instead of loop - 1) grow 2) fixUp 3) relocate - and then we can add If these all make sense I'll propose the PR. If there's something major I am missing here, I'd love to see your fix being landed to Janino and released. Thanks in advance! |
|
disassembled-gencode-janino-3-1-1-snapshot.log Also attached the disassembled gencode while running the code with Janino 3.1.1-SNAPSHOT. It doesn't throw exception (as flowAnalysis has been removed), but there's no code in "expand_doConsume" as well as no "tableswitch" is presented in the log. It would be nice to avoid crashing in runtime - is there any chance to bring flowAnalysis back, or put alternative verification? |
…up, and relocate Also add a reproducer for janino-compiler#113
|
Your disagnosis is exactly right, and I just merged your PR #114 that fixes the problem. Lucky me that my recent massive refactoring was complete before you started developing the fix ;-) ... |
|
Great! Do you have any remaining works before releasing? Since we've affected by this, I'd like to see the new release sooner. (Honestly Spark community is depending on 3.0.x and new 3.0.x release would be easier to consume, but looks like Janino does linear release with single version line so OK.) Btw, I like the way Janino does some verification and throws compilation error, like flowAnalysis for this case. Could we please reconsider restoring verification logic on the new code? |
|
A release would not be a problem at all... maybe I can do it today! |
|
Regarding "flowAnalysis()": Its sole purpose was to calculate the The new Which reasons do you see why |
|
Please refer my previous comment: I've even observed the case where the operation(s) is(are) silently removed due to bad case, Janino bug. (In above case, the content of static method I'm not sure where it "drops" the operations (and #114 would have fixed it so we should find the place without applying #114) but I'd like to see Janino throw error instead of producing unintended output. "Fail to compile" is the thing we can be noticed early and try to investigate. "Run and produce wrong result" is hard to find out and once we find out it have been already impacting the business badly. |
|
The compiler is the one we heavily rely on while we use static type languages to avoid as many possible errors as possible in runtime. This was from Janino bug so may not easy to assert from itself, but whenever possible or in doubt it would be nice to throw out error instead of taking risk of going forward. I'll recheck whether my comment is a red-herring; if it's not a red-herring then I'd like to see how we can assert it and prevent dropping out operations. |
|
Version 3.1.1 is out the door! |
|
Producing loadable but incorrect code is certainly a no-no. Can you provide a unit test that reproduces the problem with JANINO 3.1.1? And in a new issue, please... |
Great! Thanks for the quick release! I guess it would be seen in couple of days afterwards in Nexus; once
Sure! I didn't intend to extend this issue. I'll close this and file a new issue. I'm not sure I can find a new reproducer though - for now I still need to trigger a bug #113 to reproduce the case, which means it cannot be provided with unit test. (And then not against Janino 3.1.1... I don't think it becomes to be invalid - it may be the way we prevent another possible bugs.) |
### What changes were proposed in this pull request? This PR(SPARK-31101) proposes to upgrade Janino to 3.0.16 which is released recently. * Merged pull request janino-compiler/janino#114 "Grow the code for relocatables, and do fixup, and relocate". Please see the commit log. - https://github.com/janino-compiler/janino/commits/3.0.16 You can see the changelog from the link: http://janino-compiler.github.io/janino/changelog.html / though release note for Janino 3.0.16 is actually incorrect. ### Why are the changes needed? We got some report on failure on user's query which Janino throws error on compiling generated code. The issue is here: janino-compiler/janino#113 It contains the information of generated code, symptom (error), and analysis of the bug, so please refer the link for more details. Janino 3.0.16 contains the PR janino-compiler/janino#114 which would enable Janino to succeed to compile user's query properly. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Existing UTs. Closes #27932 from HeartSaVioR/SPARK-31101-janino-3.0.16. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
### What changes were proposed in this pull request? This PR(SPARK-31101) proposes to upgrade Janino to 3.0.16 which is released recently. * Merged pull request janino-compiler/janino#114 "Grow the code for relocatables, and do fixup, and relocate". Please see the commit log. - https://github.com/janino-compiler/janino/commits/3.0.16 You can see the changelog from the link: http://janino-compiler.github.io/janino/changelog.html / though release note for Janino 3.0.16 is actually incorrect. ### Why are the changes needed? We got some report on failure on user's query which Janino throws error on compiling generated code. The issue is here: janino-compiler/janino#113 It contains the information of generated code, symptom (error), and analysis of the bug, so please refer the link for more details. Janino 3.0.16 contains the PR janino-compiler/janino#114 which would enable Janino to succeed to compile user's query properly. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Existing UTs. Closes apache#27932 from HeartSaVioR/SPARK-31101-janino-3.0.16. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
This PR(SPARK-31101) proposes to upgrade Janino to 3.0.16 which is released recently. * Merged pull request janino-compiler/janino#114 "Grow the code for relocatables, and do fixup, and relocate". Please see the commit log. - https://github.com/janino-compiler/janino/commits/3.0.16 You can see the changelog from the link: http://janino-compiler.github.io/janino/changelog.html / though release note for Janino 3.0.16 is actually incorrect. We got some report on failure on user's query which Janino throws error on compiling generated code. The issue is here: janino-compiler/janino#113 It contains the information of generated code, symptom (error), and analysis of the bug, so please refer the link for more details. Janino 3.0.16 contains the PR janino-compiler/janino#114 which would enable Janino to succeed to compile user's query properly. No. Existing UTs. Closes apache#27932 from HeartSaVioR/SPARK-31101-janino-3.0.16. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
### What changes were proposed in this pull request? This PR(SPARK-31101) proposes to upgrade Janino to 3.0.16 which is released recently. * Merged pull request janino-compiler/janino#114 "Grow the code for relocatables, and do fixup, and relocate". Please see the commit log. - https://github.com/janino-compiler/janino/commits/3.0.16 You can see the changelog from the link: http://janino-compiler.github.io/janino/changelog.html / though release note for Janino 3.0.16 is actually incorrect. ### Why are the changes needed? We got some report on failure on user's query which Janino throws error on compiling generated code. The issue is here: janino-compiler/janino#113 It contains the information of generated code, symptom (error), and analysis of the bug, so please refer the link for more details. Janino 3.0.16 contains the PR janino-compiler/janino#114 which would enable Janino to succeed to compile user's query properly. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Existing UTs. Below test code fails on branch-3.0 and passes with this patch. ``` /** * NOTE: The test code tries to control the size of for/switch statement in expand_doConsume, * as well as the overall size of expand_doConsume, so that the query triggers known Janino * bug - janino-compiler/janino#113. * * The expected exception message from Janino when we use switch statement for "ExpandExec": * - "Operand stack inconsistent at offset xxx: Previous size 1, now 0" * which will not happen when we use if-else-if statement for "ExpandExec". * * "The number of fields" and "The number of distinct aggregation functions" are the major * factors to increase the size of generated code: while these values should be large enough * to trigger the Janino bug, these values should not also too big; otherwise one of below * exceptions might be thrown: * - "expand_doConsume would be beyond 64KB" * - "java.lang.ClassFormatError: Too many arguments in method signature in class file" */ test("SPARK-31115 Lots of columns and distinct aggregations shouldn't break code generation") { withSQLConf( (SQLConf.WHOLESTAGE_CODEGEN_ENABLED.key, "true"), (SQLConf.WHOLESTAGE_MAX_NUM_FIELDS.key, "10000"), (SQLConf.CODEGEN_FALLBACK.key, "false"), (SQLConf.CODEGEN_LOGGING_MAX_LINES.key, "-1") ) { var df = Seq(("1", "2", 1), ("1", "2", 2), ("2", "3", 3), ("2", "3", 4)).toDF("a", "b", "c") // The value is tested under commit "e807118eef9e0214170ff62c828524d237bd58e3": // the query fails with switch statement, whereas it passes with if-else statement. // Note that the value depends on the Spark logic as well - different Spark versions may // require different value to ensure the test failing with switch statement. val numNewFields = 100 df = df.withColumns( (1 to numNewFields).map { idx => s"a$idx" }, (1 to numNewFields).map { idx => when(col("c").mod(lit(2)).===(lit(0)), lit(idx)).otherwise(col("c")) } ) val aggExprs: Array[Column] = Range(1, numNewFields).map { idx => if (idx % 2 == 0) { coalesce(countDistinct(s"a$idx"), lit(0)) } else { coalesce(count(s"a$idx"), lit(0)) } }.toArray val aggDf = df .groupBy("a", "b") .agg(aggExprs.head, aggExprs.tail: _*) // We are only interested in whether the code compilation fails or not, so skipping // verification on outputs. aggDf.collect() } } ``` Closes #27996 from HeartSaVioR/SPARK-31101-branch-3.0. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
### What changes were proposed in this pull request? This PR(SPARK-31101) proposes to upgrade Janino to 3.0.16 which is released recently. * Merged pull request janino-compiler/janino#114 "Grow the code for relocatables, and do fixup, and relocate". Please see the commit log. - https://github.com/janino-compiler/janino/commits/3.0.16 You can see the changelog from the link: http://janino-compiler.github.io/janino/changelog.html / though release note for Janino 3.0.16 is actually incorrect. ### Why are the changes needed? We got some report on failure on user's query which Janino throws error on compiling generated code. The issue is here: janino-compiler/janino#113 It contains the information of generated code, symptom (error), and analysis of the bug, so please refer the link for more details. Janino 3.0.16 contains the PR janino-compiler/janino#114 which would enable Janino to succeed to compile user's query properly. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Existing UTs. Below test code fails on branch-2.4 and passes with this patch. (Note that there seems to be the case where another UT affects this UT to not fail - adding this to SQLQuerySuite won't fail this UT, but adding this to DateFunctionsSuite will fail this UT, and if you run this UT solely in SQLQuerySuite via `build/sbt "sql/testOnly *.SQLQuerySuite -- -z SPARK-31115"` then it fails.) ``` /** * NOTE: The test code tries to control the size of for/switch statement in expand_doConsume, * as well as the overall size of expand_doConsume, so that the query triggers known Janino * bug - janino-compiler/janino#113. * * The expected exception message from Janino when we use switch statement for "ExpandExec": * - "Operand stack inconsistent at offset xxx: Previous size 1, now 0" * which will not happen when we use if-else-if statement for "ExpandExec". * * "The number of fields" and "The number of distinct aggregation functions" are the major * factors to increase the size of generated code: while these values should be large enough * to trigger the Janino bug, these values should not also too big; otherwise one of below * exceptions might be thrown: * - "expand_doConsume would be beyond 64KB" * - "java.lang.ClassFormatError: Too many arguments in method signature in class file" */ test("SPARK-31115 Lots of columns and distinct aggregations shouldn't break code generation") { withSQLConf( (SQLConf.WHOLESTAGE_CODEGEN_ENABLED.key, "true"), (SQLConf.WHOLESTAGE_MAX_NUM_FIELDS.key, "10000"), (SQLConf.CODEGEN_FALLBACK.key, "false"), (SQLConf.CODEGEN_LOGGING_MAX_LINES.key, "-1") ) { var df = Seq(("1", "2", 1), ("1", "2", 2), ("2", "3", 3), ("2", "3", 4)).toDF("a", "b", "c") // The value is tested under commit "244405fe57d7737d81c34ba9e8917df6285889eb": // the query fails with switch statement, whereas it passes with if-else statement. // Note that the value depends on the Spark logic as well - different Spark versions may // require different value to ensure the test failing with switch statement. val numNewFields = 100 df = df.withColumns( (1 to numNewFields).map { idx => s"a$idx" }, (1 to numNewFields).map { idx => when(col("c").mod(lit(2)).===(lit(0)), lit(idx)).otherwise(col("c")) } ) val aggExprs: Array[Column] = Range(1, numNewFields).map { idx => if (idx % 2 == 0) { coalesce(countDistinct(s"a$idx"), lit(0)) } else { coalesce(count(s"a$idx"), lit(0)) } }.toArray val aggDf = df .groupBy("a", "b") .agg(aggExprs.head, aggExprs.tail: _*) // We are only interested in whether the code compilation fails or not, so skipping // verification on outputs. aggDf.collect() } } ``` Closes #27997 from HeartSaVioR/SPARK-31101-branch-2.4. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
### What changes were proposed in this pull request? This PR(SPARK-31101) proposes to upgrade Janino to 3.0.16 which is released recently. * Merged pull request janino-compiler/janino#114 "Grow the code for relocatables, and do fixup, and relocate". Please see the commit log. - https://github.com/janino-compiler/janino/commits/3.0.16 You can see the changelog from the link: http://janino-compiler.github.io/janino/changelog.html / though release note for Janino 3.0.16 is actually incorrect. ### Why are the changes needed? We got some report on failure on user's query which Janino throws error on compiling generated code. The issue is here: janino-compiler/janino#113 It contains the information of generated code, symptom (error), and analysis of the bug, so please refer the link for more details. Janino 3.0.16 contains the PR janino-compiler/janino#114 which would enable Janino to succeed to compile user's query properly. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Existing UTs. Closes apache#27932 from HeartSaVioR/SPARK-31101-janino-3.0.16. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
### What changes were proposed in this pull request? This PR proposes to upgrade Janino to 3.1.2 which is released recently. Major changes were done for refactoring, as well as there're lots of "no commit message". Belows are the pairs of (commit title, commit) which seem to deal with some bugs or specific improvements (not purposed to refactor) after 3.0.15. * Issue #119: Guarantee executing popOperand() in popUninitializedVariableOperand() via moving popOperand() out of "assert" * Issue #116: replace operand to final target type if boxing conversion and widening reference conversion happen together * Merged pull request `#114` "Grow the code for relocatables, and do fixup, and relocate". * janino-compiler/janino@367c58e * issue `#107`: Janino requires "org.codehaus.commons.compiler.io", but commons-compiler does not export this package * janino-compiler/janino@f7d9959 * Throw an NYI CompileException when a static interface method is invoked. * janino-compiler/janino@efd3884 * Fixed the promotion of the array access index expression (see JLS7 15.13 Array Access Expressions) * janino-compiler/janino@32fdb5f * Issue `#104`: ClassLoaderIClassLoader 's ClassNotFoundException handle mechanism enhancement * janino-compiler/janino@6e8a97d You can see the changelog from the link: http://janino-compiler.github.io/janino/changelog.html ### Why are the changes needed? We got some report on failure on user's query which Janino throws error on compiling generated code. The issue is here: janino-compiler/janino#113 It contains the information of generated code, symptom (error), and analysis of the bug, so please refer the link for more details. Janino 3.1.1 contains the PR janino-compiler/janino#114 which would enable Janino to succeed to compile user's query properly. I've also fixed a couple of more bugs as 3.1.1 made Spark UTs fail - hence we need to upgrade to 3.1.2. Furthermore, from my testing, janino-compiler/janino#90 (which Josh Rosen filed before) seems to be also resolved in 3.1.2 as well. Looks like Janino is maintained by one person and there's no even version branches and releases/tags so we can't expect Janino maintainer to release a new bugfix version - hence have to try out new minor version. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Existing UTs. Closes #27860 from HeartSaVioR/SPARK-31101. Authored-by: Jungtaek Lim (HeartSaVioR) <kabhwan.opensource@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
I have observed some compilation error on generated code for Spark, which is around switch statement.
(The generated code is compiled against Janino 3.0.8 but upgrading version to Janino 3.0.15 didn't seem to fix the issue.)
With some investigation I found that there's a "branch" before TABLESWITCH OP which requires "growing" - so below situation is happening:
fixUpAndRelocate1.1) The padding for TABLESWITCH is applied first in
fixUp.1.2) In
relocate, some branch requires "growing" and insert some spaces "before" TABLESWITCH. In result, TABLESWITCH OP and padded offsets are shifted, which "may" break padding.fixUpAndRelocate2.1) The padding for TABLESWITCH is applied AGAIN in
fixUp, which adds more spaces if padding was broken in 1.2)fixUpAndRelocateis finished andflowAnalysisrunsflowAnalysisreads the part ofdefaultfor TABLESWITCH which is 0 due to "double padding", hence the offset has been same as TABLESWITCH OP, and error is thrown.error-codegen.log
disassembled-gencode.log
The text was updated successfully, but these errors were encountered: