"Incompatible expression types" or verification errors when using ternary expressions with null in one branch #90
Labels
Comments
JoshRosen
changed the title
aunkrig
added a commit
that referenced
this issue
May 20, 2019
…n using ternary expressions with null in one branch
|
Thanks for your precise bug description! Actually "b ? 7 : null" is a case that is not defined in JLS7!? Now I implemented it the same way as JAVAC. Please test! |
|
Hi @aunkrig, Thanks for the quick turnaround on this fix. I tried this out with the original Spark code and have run into a new problem: You can find the generated Java code in this gist: https://gist.github.com/JoshRosen/416eac9d35bd027475cc787100b0bfe5 Line 100 is where the ternary expression fix was needed: https://gist.github.com/JoshRosen/416eac9d35bd027475cc787100b0bfe5#file-janino_90-java-L100 |
|
That's weird... according to the JVMS, the |
|
Here's Maybe it's a problem with how I did a local Instead, maybe I should set |
|
You may (experimentally) run your JVM with |
|
Ping. |
|
Spark recently bumped its Janino version to 3.0.13, which contains this fix, but unfortunately it doesn't look like this resolves my issue. I now get a new exception like This happens when I change the code generator to diff --git a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ScalaUDF.scala b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ScalaUDF.scala
index 3274b66e98..27d08ead7c 100644
--- a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ScalaUDF.scala
+++ b/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ScalaUDF.scala
@@ -1007,10 +1007,8 @@ case class ScalaUDF(
case ((eval, i), dt) =>
val argTerm = ctx.freshName("arg")
val initArg = if (CatalystTypeConverters.isPrimitive(dt)) {
- val convertedTerm = ctx.freshName("conv")
s"""
- |${CodeGenerator.boxedType(dt)} $convertedTerm = ${eval.value};
- |Object $argTerm = ${eval.isNull} ? null : $convertedTerm;
+ |Object $argTerm = ${eval.isNull} ? null : ${eval.value};
""".stripMargin
} else {
s"Object $argTerm = ${eval.isNull} ? null : $convertersTerm[$i].apply(${eval.value});" |
|
Would you please set a system property |
It's easily reproducible with below test code: which fails on 3.0.15 with same error message, and passes on master branch. I guess compiler has to deal with auto boxing which was missing on 3.0.15 and somehow addressed recently. If we don't have some test for this, I feel it would be good to have one to prevent regression. WDYT? |
|
@JoshRosen FYI the problem seems to be fixed in master branch of Janino - your patch seems to be simple so just ran with recent master branch of Spark & Janino, and it passed. I feel Spark community would like to upgrade Janino to the new release anyway due to #113 - once we upgrade the Janino your patch would work. (Though I'd love to see explicit conversion as the boxed type would be completely determined by the type of constant which ends up with not-intended type; casting from right side of ternary might do the same safely.) |
dongjoon-hyun
pushed a commit
to apache/spark
that referenced
this issue
May 29, 2020
### 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) <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Attempting to compile
with
ScriptEvaluatorresults inwith Janino 3.0.12.
Also, compiling
generates bytecode that triggers a verification error:
This is a simplified version of a problem that we encountered when generating code in Apache Spark: apache/spark#24636 (comment)
The text was updated successfully, but these errors were encountered: