[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213567775
  
Test PASSed.
Refer to this link for build results (access rights to CI server needed): 
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/56715/
Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213567771
  
Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213567556
  
**[Test build #56715 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/56715/consoleFull)**
 for PR 12600 at commit 
[`e00d80c`](https://github.com/apache/spark/commit/e00d80cd937f6217797d443b034aa18a5094937d).
 * This patch passes all tests.
 * This patch merges cleanly.
 * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213565318
  
Test PASSed.
Refer to this link for build results (access rights to CI server needed): 
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/56714/
Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213565316
  
Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213565062
  
**[Test build #56714 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/56714/consoleFull)**
 for PR 12600 at commit 
[`319ce96`](https://github.com/apache/spark/commit/319ce96d9be6dfedf3f94b987cb55fa55319ac19).
 * This patch passes all tests.
 * This patch merges cleanly.
 * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/12600


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[davies]

Github user davies commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213562402
  
Merging this into master, thanks!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213562111
  
Test PASSed.
Refer to this link for build results (access rights to CI server needed): 
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/56713/
Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213562108
  
Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213561607
  
**[Test build #56713 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/56713/consoleFull)**
 for PR 12600 at commit 
[`a5e93e6`](https://github.com/apache/spark/commit/a5e93e601998caecd66adbd2bc2f7276cde1c251).
 * This patch passes all tests.
 * This patch merges cleanly.
 * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213542932
  
**[Test build #56715 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/56715/consoleFull)**
 for PR 12600 at commit 
[`e00d80c`](https://github.com/apache/spark/commit/e00d80cd937f6217797d443b034aa18a5094937d).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[zsxwing]

Github user zsxwing commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213539712
  
Please add comments to `doPrepare` to mention the lock. Otherwise LGTM


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213537141
  
**[Test build #56714 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/56714/consoleFull)**
 for PR 12600 at commit 
[`319ce96`](https://github.com/apache/spark/commit/319ce96d9be6dfedf3f94b987cb55fa55319ac19).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[zsxwing]

Github user zsxwing commented on a diff in the pull request:

https://github.com/apache/spark/pull/12600#discussion_r60779639
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala ---
@@ -187,11 +187,11 @@ abstract class SparkPlan extends QueryPlan[SparkPlan] 
with Logging with Serializ
   /**
* Prepare a SparkPlan for execution. It's idempotent.
*/
-  final def prepare(): Unit = {
+  final def prepare(): Unit = synchronized {
--- End diff --

Could you add comments for `doPrepare` to mention the lock?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[zsxwing]

Github user zsxwing commented on a diff in the pull request:

https://github.com/apache/spark/pull/12600#discussion_r60779425
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala ---
@@ -187,11 +187,11 @@ abstract class SparkPlan extends QueryPlan[SparkPlan] 
with Logging with Serializ
   /**
* Prepare a SparkPlan for execution. It's idempotent.
*/
-  final def prepare(): Unit = {
+  final def prepare(): Unit = synchronized {
 if (prepareCalled.compareAndSet(false, true)) {
--- End diff --

nit: `prepareCalled` can be a Boolean since it's in `synchronized`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213531619
  
**[Test build #56713 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/56713/consoleFull)**
 for PR 12600 at commit 
[`a5e93e6`](https://github.com/apache/spark/commit/a5e93e601998caecd66adbd2bc2f7276cde1c251).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[zsxwing]

Github user zsxwing commented on a diff in the pull request:

https://github.com/apache/spark/pull/12600#discussion_r60778555
  
--- Diff: 
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlan.scala ---
@@ -187,11 +187,11 @@ abstract class SparkPlan extends QueryPlan[SparkPlan] 
with Logging with Serializ
   /**
* Prepare a SparkPlan for execution. It's idempotent.
*/
-  final def prepare(): Unit = {
+  final def prepare(): Unit = synchronized {
--- End diff --

This looks dangerous. Child's `doPrepare` is wrapped by `parent's 
synchronized` without any comment mentioning it. If Child's `doPrepare` starts 
a new thread and calls `prepare` in it, we may have some dead-lock race.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[zsxwing]

Github user zsxwing commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213529482
  
So `SparkPlan` must be thread-safe? Looks `subqueryResults` is not 
protected.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213287306
  
Merged build finished. Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[AmplabJenkins]

Github user AmplabJenkins commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213287309
  
Test PASSed.
Refer to this link for build results (access rights to CI server needed): 
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/56648/
Test PASSed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213286426
  
**[Test build #56648 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/56648/consoleFull)**
 for PR 12600 at commit 
[`e5bd221`](https://github.com/apache/spark/commit/e5bd221eccee7d0194c691519d65c3a88c168578).
 * This patch passes all tests.
 * This patch merges cleanly.
 * This patch adds no public classes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[rxin]

Github user rxin commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213257569
  
Is there a way to create a test case?



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[SparkQA]

Github user SparkQA commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213257260
  
**[Test build #56648 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/56648/consoleFull)**
 for PR 12600 at commit 
[`e5bd221`](https://github.com/apache/spark/commit/e5bd221eccee7d0194c691519d65c3a88c168578).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[davies]

Github user davies commented on the pull request:

https://github.com/apache/spark/pull/12600#issuecomment-213257084
  
cc @zsxwing @rxin


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request: [SPARK-14791] [SQL] fix risk condition between...

[davies]

GitHub user davies opened a pull request:

https://github.com/apache/spark/pull/12600

[SPARK-14791] [SQL] fix risk condition between broadcast and subquery

## What changes were proposed in this pull request?

SparkPlan.prepare() could be called in different threads (BroadcastExchange 
will call it in a thread pool), it only make sure that doPrepare() will only be 
called once, the second call to prepare() may return earlier before all the 
children had finished prepare(). Then some operator may call doProduce() before 
prepareSubqueries(), `null` will be used as the result of subquery, which is 
wrong. This cause TPCDS Q23B returns wrong answer sometimes.

This PR added synchronization for prepare(), make sure all the children had 
finished prepare() before return. Also call prepare() in produce() (similar to 
execute()).

Added checking for ScalarSubquery to make sure that the subquery has 
finished before using the result.

## How was this patch tested?

Manually tested with Q23B, no wrong answer anymore.




You can merge this pull request into a Git repository by running:

$ git pull https://github.com/davies/spark fix_risk

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/12600.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #12600


commit e5bd221eccee7d0194c691519d65c3a88c168578
Author: Davies Liu 
Date:   2016-04-22T04:52:36Z

fix risk condition between broadcast and subquery




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org