[SPARK-29128][SQL] Split predicate code in OR expressions by maropu · Pull Request #25827 · apache/spark

maropu · 2019-09-18T05:05:22Z

What changes were proposed in this pull request?

This pr is to split predicate code in OR expressions. When I checked if method bytecode size in BenchmarkQueryTest went over the OpenJDK default limit (8000) or not in #25788, I found TPCDSQuerySuite.modified-q3 had too big functions. That's because too long OR chains in the query generate too long code in a single function;

modified-q3 generates the code below in the master

== Subtree 2 / 4 (maxMethodCodeSize:12497; maxConstantPoolSize:732(1.12% used); numInnerClasses:1) ==
^^^^^^
*(3) HashAggregate(keys=[d_year#9, i_brand#62, i_brand_id#61], functions=[partial_sum(UnscaledValue(ss_net_profit#53))], output=[d_year#9, i_brand#62, i_brand_id#61, sum#85L])
+- *(3) Project [d_year#9, ss_net_profit#53, i_brand_id#61, i_brand#62]
....
/* 365 */ private void agg_doAggregateWithKeys_0() throws java.io.IOException {
/* 366 */ if (columnartorow_mutableStateArray_1[0] == null) {
/* 367 */ columnartorow_nextBatch_0();
/* 368 */ }
/* 369 */ while ( columnartorow_mutableStateArray_1[0] != null) {
/* 370 */ int columnartorow_numRows_0 = columnartorow_mutableStateArray_1[0].numRows();
/* 371 */ int columnartorow_localEnd_0 = columnartorow_numRows_0 - columnartorow_batchIdx_0;
/* 372 */ for (int columnartorow_localIdx_0 = 0; columnartorow_localIdx_0 < columnartorow_localEnd_0; columnartorow_localIdx_0++) {
/* 373 */ int columnartorow_rowIdx_0 = columnartorow_batchIdx_0 + columnartorow_localIdx_0;
/* 374 */ do {
/* 375 */ boolean columnartorow_isNull_0 = columnartorow_mutableStateArray_2[0].isNullAt(columnartorow_rowIdx_0);
/* 376 */ int columnartorow_value_0 = columnartorow_isNull_0 ? -1 : (columnartorow_mutableStateArray_2[0].getInt(columnartorow_rowIdx_0));
/* 377 */
/* 378 */ boolean filter_value_2 = !columnartorow_isNull_0;
/* 379 */ if (!filter_value_2) continue;
/* 380 */
/* 381 */ boolean filter_value_12 = false;
/* 382 */ filter_value_12 = columnartorow_value_0 >= 2415355;
/* 383 */ boolean filter_value_11 = false;
/* 384 */
.....
too long code

This pr split the predicate code into small functions below;

== Subtree 2 / 4 (maxMethodCodeSize:688; maxConstantPoolSize:949(1.45% used); numInnerClasses:1) ==
^^^^
*(3) HashAggregate(keys=[d_year#9, i_brand#62, i_brand_id#61], functions=[partial_sum(UnscaledValue(ss_net_profit#53))], output=[d_year#9, i_brand#62, i_brand_id#61, sum#85L])
+- *(3) Project [d_year#9, ss_net_profit#53, i_brand_id#61, i_brand#62]
...
/* 3285 */ private void agg_doAggregateWithKeys_0() throws java.io.IOException {
/* 3286 */ if (columnartorow_mutableStateArray_1[0] == null) {
/* 3287 */ columnartorow_nextBatch_0();
/* 3288 */ }
/* 3289 */ while ( columnartorow_mutableStateArray_1[0] != null) {
/* 3290 */ int columnartorow_numRows_0 = columnartorow_mutableStateArray_1[0].numRows();
/* 3291 */ int columnartorow_localEnd_0 = columnartorow_numRows_0 - columnartorow_batchIdx_0;
/* 3292 */ for (int columnartorow_localIdx_0 = 0; columnartorow_localIdx_0 < columnartorow_localEnd_0; columnartorow_localIdx_0++) {
/* 3293 */ int columnartorow_rowIdx_0 = columnartorow_batchIdx_0 + columnartorow_localIdx_0;
/* 3294 */ do {
/* 3295 */ boolean columnartorow_isNull_0 = columnartorow_mutableStateArray_2[0].isNullAt(columnartorow_rowIdx_0);
/* 3296 */ int columnartorow_value_0 = columnartorow_isNull_0 ? -1 : (columnartorow_mutableStateArray_2[0].getInt(columnartorow_rowIdx_0));
/* 3297 */
/* 3298 */ boolean filter_value_2 = !columnartorow_isNull_0;
/* 3299 */ if (!filter_value_2) continue;
/* 3300 */
/* 3301 */ boolean filter_value_213 = filter_or_8(columnartorow_value_0, columnartorow_isNull_0);
/* 3302 */ boolean filter_value_5 = true;
/* 3303 */
/* 3304 */ if (!filter_value_213) {
/* 3305 */ boolean filter_value_421 = filter_or_17(columnartorow_value_0, columnartorow_isNull_0);
/* 3306 */ filter_value_5 = filter_value_421;
/* 3307 */ }
/* 3308 */ boolean filter_value_4 = true;
/* 3309 */
/* 3310 */ if (!filter_value_5) {
/* 3311 */ boolean filter_value_630 = filter_or_26(columnartorow_value_0, columnartorow_isNull_0);
/* 3312 */ boolean filter_value_422 = true;
/* 3313 */
/* 3314 */ if (!filter_value_630) {
/* 3315 */ boolean filter_value_838 = filter_or_35(columnartorow_value_0, columnartorow_isNull_0);
/* 3316 */ filter_value_422 = filter_value_838;
/* 3317 */ }
/* 3318 */ filter_value_4 = filter_value_422;
/* 3319 */ }
/* 3320 */ boolean filter_value_3 = true;
/* 3321 */
/* 3322 */ if (!filter_value_4) {
/* 3323 */ boolean filter_value_1048 = filter_or_44(columnartorow_value_0, columnartorow_isNull_0);
/* 3324 */ boolean filter_value_840 = true;
/* 3325 */
/* 3326 */ if (!filter_value_1048) {
/* 3327 */ boolean filter_value_1256 = filter_or_53(columnartorow_value_0, columnartorow_isNull_0);
/* 3328 */ filter_value_840 = filter_value_1256;
/* 3329 */ }
/* 3330 */ boolean filter_value_839 = true;
/* 3331 */
/* 3332 */ if (!filter_value_840) {
/* 3333 */ boolean filter_value_1465 = filter_or_62(columnartorow_value_0, columnartorow_isNull_0);
/* 3334 */ boolean filter_value_1257 = true;
/* 3335 */
/* 3336 */ if (!filter_value_1465) {
/* 3337 */ boolean filter_value_1673 = filter_or_71(columnartorow_value_0, columnartorow_isNull_0);
/* 3338 */ filter_value_1257 = filter_value_1673;
/* 3339 */ }
/* 3340 */ filter_value_839 = filter_value_1257;
/* 3341 */ }
/* 3342 */ filter_value_3 = filter_value_839;
/* 3343 */ }
/* 3344 */ if (!filter_value_3) continue;
/* 3345 */
/* 3346 */ boolean columnartorow_isNull_1 = columnartorow_mutableStateArray_2[1].isNullAt(columnartorow_rowIdx_0);
/* 3347 */ int columnartorow_value_1 = columnartorow_isNull_1 ? -1 : (columnartorow_mutableStateArray_2[1].getInt(columnartorow_rowIdx_0));
/* 3348 */
/* 3349 */ boolean filter_value_1676 = !columnartorow_isNull_1;
/* 3350 */ if (!filter_value_1676) continue;
/* 3351 */

Why are the changes needed?

For better generated code.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing unit tests.

maropu · 2019-09-18T05:09:35Z

+ // of the whole stage codegen. But, we don't do so now because the performance changes that
+ // we don't expect might occur in many queries. Therefore, we currently apply
+ // this split function to specific performance-sensitive places only,
+ // e.g., common subexpression elimination for the whole stage codegen and OR expressions.


@viirya @cloud-fan Is this correct? I remember @viirya worked on this in #21140 long before, but the pr wasn't merged because of some reasons: performance or design issues?

I recall that is because string-based manipulation was thought too buggy? I didn't remember it is because of performance issue.

Aha, I see.

SparkQA · 2019-09-18T07:05:02Z

Test build #110865 has finished for PR 25827 at commit d116613.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2019-09-18T07:09:57Z

retest this please

kiszk · 2019-09-18T09:29:06Z

+ * Defines an independent function for a given expression and returns a caller-side code
+ * for the function as `ExprCode`.
+ */
+ def defineIndependentFunction(


What does Independent stand for?

Ur, I made a mistake trying to write Individual.. I'll update soon.

SparkQA · 2019-09-18T12:17:34Z

Test build #110873 has finished for PR 25827 at commit d116613.

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

SparkQA · 2019-09-19T07:05:02Z

Test build #110967 has finished for PR 25827 at commit 7d8a382.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2019-09-19T07:06:23Z

retest this please

SparkQA · 2019-09-19T12:15:07Z

Test build #110975 has finished for PR 25827 at commit 7d8a382.

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

maropu · 2019-09-20T00:09:30Z

Anyone could check this? @cloud-fan @viirya @kiszk

kiszk · 2019-09-20T07:10:28Z

 }

+ /**
+ * Defines an independent function for a given expression and returns a caller-side code


nit: is it better to update independent?

kiszk

except one nit

viirya · 2019-09-20T21:36:02Z

 private def reduceCodeSize(ctx: CodegenContext, eval: ExprCode): Unit = {
 // TODO: support whole stage codegen too
+ //
+ // NOTE: We could use `CodeGenerator.defineSingleSplitFunction` here for the code path


Why add this comment?

To avoid duplicate work. Currently, this func and defineSingleSplitFunction have too similar logic.

viirya · 2019-09-20T21:40:09Z

+ ev: ExprCode,
+ funcNameOption: Option[String] = None,
+ inputVarsOption: Option[Seq[VariableValue]] = None): ExprCode = {
+ val inputVars = inputVarsOption.getOrElse(getLocalInputVariableValues(this, expr).toSeq)


Actually, I am not sure if getLocalInputVariableValues is totally ok to extract all input variables from an expr. This is what I want to do when I was changing codegen. I remember not all expressions are changed to expose input variables information. I did the change but stoped in the middle because the community has different idea other than this string based framework.

Ur, I see. we might need more discussion around this code. If that's true, it might be dangerous to use this in splitAggrExpr and common subexpr elimination, too.

SparkQA · 2019-09-21T04:35:53Z

Test build #111099 has finished for PR 25827 at commit ca6522a.

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

SparkQA · 2019-10-26T15:57:54Z

Test build #112716 has finished for PR 25827 at commit ab15e24.

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

SparkQA · 2019-11-21T04:48:04Z

Test build #114185 has finished for PR 25827 at commit 543c016.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2019-11-21T04:51:30Z

retest this please

SparkQA · 2019-11-21T05:11:35Z

Test build #114184 has finished for PR 25827 at commit 47e28c4.

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

cloud-fan · 2019-11-21T06:26:02Z

hmm is it a common problem for all expressions? e.g. the children are very big expression trees.

maropu · 2019-11-21T06:38:59Z

yea, I think so. @viirya worked on that issue a few years ago (IIRC that work was not merged cuz we couldn't reach a consensus for general solution). This pr targets to solve a part of the issue caused in OR expressions only.

SparkQA · 2019-11-21T14:17:36Z

Test build #114230 has finished for PR 25827 at commit 543c016.

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

maropu · 2019-12-13T05:34:46Z

retest this please

SparkQA · 2019-12-13T08:05:02Z

Test build #115280 has finished for PR 25827 at commit 543c016.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2019-12-13T13:00:51Z

retest this please

SparkQA · 2019-12-13T16:59:38Z

Test build #115303 has finished for PR 25827 at commit 543c016.

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

maropu · 2020-01-15T07:35:25Z

retest this please

SparkQA · 2020-01-15T08:05:02Z

Test build #116761 has finished for PR 25827 at commit 543c016.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2020-01-16T07:43:12Z

retest this please

SparkQA · 2020-01-16T08:05:01Z

Test build #116824 has finished for PR 25827 at commit 543c016.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-01-16T10:01:09Z

retest this please

SparkQA · 2020-01-16T14:49:13Z

Test build #116847 has finished for PR 25827 at commit 543c016.

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

maropu · 2020-02-07T13:03:49Z

retest this please

SparkQA · 2020-02-07T17:04:13Z

Test build #118037 has finished for PR 25827 at commit 543c016.

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

maropu · 2020-02-16T23:32:13Z

retest this please

SparkQA · 2020-02-17T04:01:50Z

Test build #118514 has finished for PR 25827 at commit 543c016.

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

maropu · 2020-02-27T15:59:19Z

retest this please

SparkQA · 2020-02-27T21:00:32Z

Test build #119045 has finished for PR 25827 at commit 543c016.

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

maropu · 2020-03-16T02:38:11Z

retest this please

SparkQA · 2020-03-16T06:34:18Z

Test build #119824 has finished for PR 25827 at commit 543c016.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-03-16T07:32:04Z

retest this please

SparkQA · 2020-03-16T13:44:05Z

Test build #119848 has finished for PR 25827 at commit 543c016.

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

maropu · 2020-03-25T12:51:07Z

retest this please

SparkQA · 2020-03-25T13:20:48Z

Test build #120355 has finished for PR 25827 at commit 543c016.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-03-25T22:48:47Z

retest this please

SparkQA · 2020-03-26T03:52:09Z

Test build #120380 has finished for PR 25827 at commit 543c016.

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

github-actions · 2020-07-05T00:31:00Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

kiszk · 2020-07-05T07:16:36Z

retest this please

SparkQA · 2020-07-06T02:52:09Z

Test build #124959 has finished for PR 25827 at commit 543c016.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

maropu commented Sep 18, 2019
View reviewed changes

kiszk reviewed Sep 18, 2019
View reviewed changes

dongjoon-hyun added the SQL label Sep 18, 2019

maropu force-pushed the SplitOrPredicate branch from d116613 to 7d8a382Compare September 19, 2019 03:52

kiszk reviewed Sep 20, 2019
View reviewed changes

kiszk approved these changes Sep 20, 2019
View reviewed changes

viirya reviewed Sep 20, 2019
View reviewed changes

maropu mentioned this pull request Sep 21, 2019
[SPARK-29084][SQL][TESTS] Check method bytecode size in BenchmarkQueryTest #25788
Closed

maropu force-pushed the SplitOrPredicate branch from ca6522a to ab15e24Compare October 26, 2019 12:17

maropu added 3 commits November 21, 2019 10:13

Fix
0283971

Fix
9969494

Fix
c5b203a

maropu force-pushed the SplitOrPredicate branch from ab15e24 to 47e28c4Compare November 21, 2019 01:14

Fix
543c016

maropu force-pushed the SplitOrPredicate branch from 47e28c4 to 543c016Compare November 21, 2019 01:16

github-actionsBot added the Stale label Jul 5, 2020

github-actionsBot closed this Jul 6, 2020

Uh oh!

Conversation

maropu commented Sep 18, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 18, 2019

Uh oh!

maropu commented Sep 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 18, 2019

Uh oh!

SparkQA commented Sep 19, 2019

Uh oh!

maropu commented Sep 19, 2019

Uh oh!

SparkQA commented Sep 19, 2019

Uh oh!

maropu commented Sep 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropuSep 20, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 21, 2019

Uh oh!

SparkQA commented Oct 26, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

maropu commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

cloud-fan commented Nov 21, 2019

Uh oh!

maropu commented Nov 21, 2019

Uh oh!

SparkQA commented Nov 21, 2019

Uh oh!

maropu commented Dec 13, 2019

Uh oh!

SparkQA commented Dec 13, 2019

Uh oh!

maropu commented Dec 13, 2019

Uh oh!

SparkQA commented Dec 13, 2019

Uh oh!

maropu commented Jan 15, 2020

Uh oh!

SparkQA commented Jan 15, 2020

Uh oh!

kiszk commented Jan 16, 2020

Uh oh!

SparkQA commented Jan 16, 2020

Uh oh!

maropu commented Jan 16, 2020

maropuSep 20, 2019•
edited
Loading