Uh oh!
There was an error while loading. Please reload this page.
[WIP][SPARK-29221][SQL] LocalTableScanExec: handle the case where executors are accessing "null" rows - #25913
[WIP][SPARK-29221][SQL] LocalTableScanExec: handle the case where executors are accessing "null" rows#25913HeartSaVioR wants to merge 2 commits into
Conversation
HeartSaVioR
commented
Sep 24, 2019
I'll remove [WIP] once the 5 sequential tests will not fail on |
HeartSaVioR
commented
Sep 24, 2019
retest this, please |
3 similar comments
HeartSaVioR
commented
Sep 24, 2019
retest this, please |
HeartSaVioR
commented
Sep 24, 2019
retest this, please |
HeartSaVioR
commented
Sep 24, 2019
retest this, please |
SparkQA
commented
Sep 24, 2019
Test build #111273 has finished for PR 25913 at commit
|
NPE got resolved in LocalTableScanExec, but further step triggers another exception: and actual stack trace is |
I guess the approach should be changed, as there seems to be plenty of pieces in physical plans which are not safe to execute in executors. Something might seem to be changed recently. spark/sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala Lines 976 to 983 in fff2e84 When values/methods in SparkPlan are referenced in executor side, |
SparkQA
commented
Sep 24, 2019
Test build #111274 has finished for PR 25913 at commit
|
SparkQA
commented
Sep 24, 2019
Test build #111275 has finished for PR 25913 at commit
|
SparkQA
commented
Sep 24, 2019
Test build #111276 has finished for PR 25913 at commit
|
| @transient private lazy val unsafeRows: Array[InternalRow] = { | ||
| if (rows.isEmpty) { | ||
| if (rows == null || rows.isEmpty) { |
There was a problem hiding this comment.
We cannot remove @transient instead of this?
There was a problem hiding this comment.
Adding transient is to avoid serializing and copying whole rows, so I guess it's preferred one.
There was a problem hiding this comment.
Ah, I see. I'm a bit worried that this value could be different between a driver and executors. Which code touches this variable in executors? What's the stacktrace for unsafeRows?
There was a problem hiding this comment.
Uh, that's actually a kind of defensive programming, as it would bring NPE if there's no check for null. I haven't seen any actual stack trace to refer to unsafeRows.
There was a problem hiding this comment.
I personally think unsafeRows is not expected to be called in executor sides, so we need to check assert(rows != null) instead of the current approach. If it fails then, we need to fix some code not to call this variable in executors.
SparkQA
commented
Sep 24, 2019
Test build #111277 has finished for PR 25913 at commit
|
HeartSaVioR
commented
Sep 24, 2019
It would be nice if someone in Spark SQL expert in depth could spend some time to investigate origin issue. I could apply sequential of band-aids until the further error message is gone, but I feel there might be another root issue on this. |
HeartSaVioR
commented
Sep 24, 2019
Just updated the observation of second kind of error pattern. |
Triggering new 3 builds to see the chance of encountering new occurrence. |
HeartSaVioR
commented
Sep 24, 2019
retest this, please |
SparkQA
commented
Sep 24, 2019
Test build #111289 has started for PR 25913 at commit |
HeartSaVioR
commented
Sep 24, 2019
retest this, please |
SparkQA
commented
Sep 24, 2019
Test build #111291 has started for PR 25913 at commit |
HeartSaVioR
commented
Sep 24, 2019
retest this, please |
SparkQA
commented
Sep 24, 2019
Test build #111292 has started for PR 25913 at commit |
HeartSaVioR
commented
Sep 24, 2019
OK all three builds were cancelled. I'll just retrigger these builds. |
HeartSaVioR
commented
Sep 24, 2019
retest this, please |
HeartSaVioR
commented
Sep 24, 2019
Just reported the build failure to dev. mailing list as it seems to be env. issue. Not all PR builds have been failed, so worth retriggering. |
HeartSaVioR
commented
Sep 24, 2019
retest this, please |
cc. @gatorsmile@cloud-fan@viirya to kindly ask for help to investigate actual cause. |
| if (rows.isEmpty) { | ||
| if (rows == null) { | ||
| Iterator("<unknown>", output) | ||
| } else if (rows.isEmpty) { |
There was a problem hiding this comment.
Is it ok to return different values in a driver and executors? How about computing this value in a driver then passing into executors?
There was a problem hiding this comment.
The value except output should be meaningless, otherwise assumption in related comments goes wrong and we should serialize the rows. I guess that's only for visual.
7079481 to
c3b59c6CompareI'm wondering why the test fails intermittently, as the stack trace should always bring NPE. My bet is that it goes to the conditional case and trigger this.
EDIT: |
SparkQA
commented
Sep 25, 2019
Test build #111309 has finished for PR 25913 at commit
|
SparkQA
commented
Sep 25, 2019
Test build #111312 has finished for PR 25913 at commit
|
SparkQA
commented
Sep 25, 2019
Test build #111311 has finished for PR 25913 at commit
|
SparkQA
commented
Sep 25, 2019
Test build #111315 has finished for PR 25913 at commit
|
viirya
commented
Sep 25, 2019
Submitted #25925 to fix it. |
HeartSaVioR
commented
Sep 25, 2019
maropu
commented
Sep 26, 2019
Anyway, nice catch, @HeartSaVioR ! |
HeartSaVioR
commented
Sep 26, 2019
Happy to help! Thanks again @viirya to deal with root cause. :) |
viirya
commented
Sep 26, 2019
Thanks @HeartSaVioR for investigating this issue! |
What changes were proposed in this pull request?
This patch proposes to fix the flaky test in
SQLQueryTestSuite.sql (subquery/scalar-subquery/scalar-subquery-select.sql), which seems to throw NPE in executor side.Both
rowsandunsafeRowsin LocalTableScanExec are defined as@transientassuming that they're not needed in executors, but according to the error message and stack trace, at leastrowshas a path to be accessed from executors.unsafeRowswill also throw NPE when executors access the value, as it accesses rows which would be null - this patch fixes it altogether.Why are the changes needed?
This patch tries to fix a flaky test.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Will trigger CI build multiple times as
build/sbt "~sql/test-only *SQLQueryTestSuite -- -z subquery/scalar-subquery/scalar-subquery-select.sql"doesn't fail in my local dev.