[SPARK-21619][SQL] Fail the execution of canonicalized plans explicitly #18828
Conversation
|
test this please |
|
cc @shaneknapp It sounds like we are unable to trigger the test. Could you please check it? |
|
Test build #80187 has finished for PR 18828 at commit
|
|
Test build #80190 has finished for PR 18828 at commit
|
|
Still looking into it, but the failure is related to reuse exchange and caching. |
| * This is used solely for making sure we wouldn't execute a canonicalized plan. | ||
| * See [[canonicalized]] on how this is set. | ||
| */ | ||
| @transient |
There was a problem hiding this comment.
I guess plans are already not valid on executors, by why @transient?
|
Test build #80276 has finished for PR 18828 at commit
|
|
Test build #80279 has finished for PR 18828 at commit
|
|
Test build #80280 has finished for PR 18828 at commit
|
|
There is another zero argument ResetCommand. Do we also need to override the makeCopy function? |
| intercept[IllegalStateException] { plan.executeCollectPublic() } | ||
| intercept[IllegalStateException] { plan.executeToIterator() } | ||
| intercept[IllegalStateException] { plan.executeBroadcast() } | ||
| intercept[IllegalStateException] { plan.executeTake(1) } |
There was a problem hiding this comment.
nit. There is an inconsistent corner case in plan.executeTake.
plan.executeTake(1) -> raise exception
plan.executeTake(0) -> no exception
plan.executeTake(-1) -> raise exceptionThere was a problem hiding this comment.
That's not an issue with this test, is it? It's just how execution is done.
|
ping @rxin |
|
Test build #83148 has finished for PR 18828 at commit
|
|
Test build #83149 has finished for PR 18828 at commit
|
|
LGTM Thanks! Merged to master. |
What changes were proposed in this pull request?
Canonicalized plans are not supposed to be executed. I ran into a case in which there's some code that accidentally calls execute on a canonicalized plan. This patch throws a more explicit exception when that happens.
How was this patch tested?
Added a test case in SparkPlanSuite.