HeartSaVioR commented on a change in pull request #27872: [SPARK-31115][SQL]
Provide config to avoid using switch statement in generated code to avoid
Janino bug
URL: https://github.com/apache/spark/pull/27872#discussion_r390810675
##########
File path:
sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala
##########
@@ -957,4 +956,61 @@ class DataFrameAggregateSuite extends QueryTest
assert(error.message.contains("function count_if requires boolean type"))
}
}
+
+ /**
+ * 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 - https://github.com/janino-compiler/janino/issues/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"),
+ (SQLConf.CODEGEN_USE_SWITCH_STATEMENT.key, "false")
+ ) {
+ 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
Review comment:
Originally I was crafting the patch against Spark 2.3 - in Spark 2.3,
setting this to 100 throws exception which is not from Janino bug, but from
either hitting 64KB limit or parameter limitation on method signature. (That's
why I added the details on exceptions when the value exceeds upper limit.)
For Spark 2.3, `70` is the thing making `switch statement` failing and `if ~
else if ~ else statement` passing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
