You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/03/26 06:20:47 UTC
[GitHub] [spark] wangshuo128 opened a new pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
wangshuo128 opened a new pull request #31968:
URL: https://github.com/apache/spark/pull/31968
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
-->
### What changes were proposed in this pull request?
In the current code, a `QueryExecution` will be wrapped in `SQLExecution.withNewExecutionId` twice when run SQL with side effects.
To reproduce:
Run SQLs below in spark-sql
```
spark-sql> CREATE TABLE t AS SELECT 1 AS i;
spark-sql> SELECT * FROM t;
spark-sql> DROP TABLE t;
```
we will see duplicate SQL queries in Spark UI :
<img width="1428" alt="0F9E09B0-7FAF-4424-9C11-66930F1BBB7B" src="https://user-images.githubusercontent.com/4003322/112590685-3435f500-8e3e-11eb-8fef-3b3b77555b0e.png">
### Why are the changes needed?
Bug fix.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
New UT and manually checked in Spark UI.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r603123418
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
##########
@@ -223,11 +224,18 @@ class Dataset[T] private[sql](
@transient private[sql] val logicalPlan: LogicalPlan = {
// For various commands (like DDL) and queries with side effects, we force query execution
// to happen right away to let these side effects take place eagerly.
+ def eagerRun(plan: LogicalPlan): LogicalPlan = {
+ val relation =
+ LocalRelation(plan.output, withAction("command", queryExecution)(_.executeCollect()))
Review comment:
Yes.
IIUC, For showing SQL queries and associated jobs in Spark UI, only tracking the SQL execution is enough.
But in general, I think it's better to track all the execution ids and let the event listeners decide the behavior for the different events for one SQL query.
So another option may be that we still track and send all the SQL execution-related events to listeners, and handle the first event of a SQL query in UI listeners.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812834091
**[Test build #136878 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136878/testReport)** for PR 31968 at commit [`4e61f11`](https://github.com/apache/spark/commit/4e61f1145386696e07019e78d99f4eb02a8461f6).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-809194919
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136642/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816610057
**[Test build #137125 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137125/testReport)** for PR 31968 at commit [`4634aee`](https://github.com/apache/spark/commit/4634aee68794ef6e5229fee24ec8b7ef92ca5e52).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812520105
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136848/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607128500
##########
File path: sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriverSuite.scala
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.hive.thriftserver
+
+import scala.collection.mutable.ListBuffer
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.scheduler.{SparkListener, SparkListenerEvent}
+import org.apache.spark.sql.execution.ui.SparkListenerSQLExecutionStart
+
+class SparkSQLDriverSuite extends SparkFunSuite {
+ private var driver: SparkSQLDriver = _
+
+ override protected def beforeEach(): Unit = {
+ System.setProperty("spark.master", "local")
+ SparkSQLEnv.init()
+ driver = new SparkSQLDriver()
+ }
+
+ override protected def afterEach(): Unit = {
+ driver.destroy()
+ driver.close()
+ SparkSQLEnv.stop()
+ }
+
+ test("Avoid wrapped in withNewExecutionId twice when run SQL with side effects") {
+ val listener = new TestListener
+ driver.context.sparkContext.addSparkListener(listener)
Review comment:
let's add a try-finally to remove the listener at the end.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r608660949
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +65,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
Review comment:
> don't create extra QueryExecution in SaprkSQLDriver.run
+1 to fix this.
One idea is to move the eager command execute to `QueryExecution`, so that `withNewExecutionId` can know if it's a command execution or not from `QueryExecution` instance.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817469235
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41772/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817612548
**[Test build #137205 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137205/testReport)** for PR 31968 at commit [`d1d0def`](https://github.com/apache/spark/commit/d1d0deff251910c45e6a2f40db6b77f219afc866).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r603123418
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
##########
@@ -223,11 +224,18 @@ class Dataset[T] private[sql](
@transient private[sql] val logicalPlan: LogicalPlan = {
// For various commands (like DDL) and queries with side effects, we force query execution
// to happen right away to let these side effects take place eagerly.
+ def eagerRun(plan: LogicalPlan): LogicalPlan = {
+ val relation =
+ LocalRelation(plan.output, withAction("command", queryExecution)(_.executeCollect()))
Review comment:
Yes.
IIUC, For showing SQL queries and associated jobs in Spark UI, only tracking the first SQL execution is enough.
But in general, I think it's better to track all the execution ids and let the event listeners decide the behavior for the different events for one SQL query.
So another option may be that we still track and send all the SQL execution-related events to listeners, and handle the first event of a SQL query in UI listeners.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r610475691
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala
##########
@@ -71,7 +72,26 @@ class QueryExecution(
lazy val analyzed: LogicalPlan = executePhase(QueryPlanningTracker.ANALYSIS) {
// We can't clone `logical` here, which will reset the `_analyzed` flag.
- sparkSession.sessionState.analyzer.executeAndCheck(logical, tracker)
+ val plan = sparkSession.sessionState.analyzer.executeAndCheck(logical, tracker)
+
+ // For various commands (like DDL) and queries with side effects, we force query execution
+ // to happen right away to let these side effects take place eagerly.
+ def runCommand(plan: LogicalPlan): LogicalPlan = {
+ val qe = new QueryExecution(sparkSession, plan, isCommand = true)
+ LocalRelation(plan.output,
+ SQLExecution.withNewExecutionId(qe)(qe.executedPlan.executeCollect()),
+ fromCommand = true)
+ }
+
+ if (isCommand) {
+ plan
+ } else {
+ plan.transformUp {
+ case c: Command => runCommand(c)
Review comment:
Do we need the `isCommand` flag? There is no nested command and it's OK to always find and run command.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812349263
**[Test build #136837 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136837/testReport)** for PR 31968 at commit [`37d64d5`](https://github.com/apache/spark/commit/37d64d53a8a59de3617b1a8114cd28d25f30c900).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816795743
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137136/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816546253
**[Test build #137125 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137125/testReport)** for PR 31968 at commit [`4634aee`](https://github.com/apache/spark/commit/4634aee68794ef6e5229fee24ec8b7ef92ca5e52).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819382965
**[Test build #137341 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137341/testReport)** for PR 31968 at commit [`50b7764`](https://github.com/apache/spark/commit/50b7764bef8c48466b426cae1e57534e4fef143c).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817486483
**[Test build #137193 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137193/testReport)** for PR 31968 at commit [`6fcdeea`](https://github.com/apache/spark/commit/6fcdeea0d3350c6247eb481ae29481a98f61777e).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812431877
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41426/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812315217
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41415/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819300614
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41912/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607812378
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +65,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
Review comment:
`toDF` only produces a `executionId` by `SQLExecution.withNewExecutionId`, but the `command` with side effects would run only once. It happens as below:
1. when run `sql("show tables")`, Spark generates a `QueryExecution` object. The `QueryExecution`'s `LogicalPlan` is eagerly executed when we pass the `QueryExecution` to a Dataset and initialize `Dataset.logicalPlan`
2. when run `show`, the code goes to `Dataset.showString`https://github.com/apache/spark/blob/3b634f66c3e4a942178a1e322ae65ce82779625d/sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala#L340
and then call `Dataset.getRows`
https://github.com/apache/spark/blob/3b634f66c3e4a942178a1e322ae65ce82779625d/sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala#L291
we new a `Dataset` with the same `QueryExecution` object in _**step1**_
https://github.com/apache/spark/blob/3b634f66c3e4a942178a1e322ae65ce82779625d/sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala#L454
thus, the `command` would be wrapped in `SQLExecution.withNewExecutionId` when initializing the `df.logicalPlan` , however, the `QueryExecution` has already been optimized and planned, won't execute the underlying command any more, see `ExecutedCommandExec.sideEffectResult`
https://github.com/apache/spark/blob/3b634f66c3e4a942178a1e322ae65ce82779625d/sql/core/src/main/scala/org/apache/spark/sql/execution/command/commands.scala#L68
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816715693
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41714/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816582238
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812396717
**[Test build #136848 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136848/testReport)** for PR 31968 at commit [`ed3b4f8`](https://github.com/apache/spark/commit/ed3b4f8c78de082a56b2f473cd025f66b6f66266).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819538022
**[Test build #137341 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137341/testReport)** for PR 31968 at commit [`50b7764`](https://github.com/apache/spark/commit/50b7764bef8c48466b426cae1e57534e4fef143c).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816676198
**[Test build #137136 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137136/testReport)** for PR 31968 at commit [`6d60b97`](https://github.com/apache/spark/commit/6d60b97c3e97fbc6e362aa89ea8503db5e8ecfa4).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r603144714
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +64,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
+ val logicalPlan = context.sql(command).logicalPlan
Review comment:
Ah I see. What do we see if we click the two UI links? Is one of them less reasonable than the other?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-809517936
> only show the DropTableCommand
This sounds better, as `LocalTableScan` is kind of disconnected from the command.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816611337
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137125/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r608490932
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +65,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
Review comment:
When run `sql("show tables").collect`, there are 2 same SQL queries in SQL UI: execute `ShowTablesCommand `. This is different from running "show tables" in spark-sql.
The reason is that:
1. run "show tables" in spark-sql
we create a DataFrame from the "show tables" SQL, its `logicalPlan` is a `LocalRelation`. Then create a `QueryExecution` from the `LocalRelation` and convert it to Hive result string.
see`SaprkSQLDriver.run`
https://github.com/apache/spark/blob/06c09a79b371c5ac3e4ebad1118ed94b460f48d1/sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala#L67-L70
the query plan is to collect from `LocalRelation`
2. run `sql.("show tables").collect`
When call collect, the `Dataset` directly use its own `QueryExecution` , see https://github.com/apache/spark/blob/06c09a79b371c5ac3e4ebad1118ed94b460f48d1/sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala#L2983
the query plan is just to execute the `ShowTablesCommand`
So it seems that we can't solve the problem by checking the `LocalRelation.fromCommand` in `SQLExecution. withNewExecutionId `
I'm thinking another way:
1. unify the behavior in `SaprkSQLDriver.run` and `Dataset.collect`
don't create extra `QueryExecution` in `SaprkSQLDriver.run` (what's the original purpose to create a new `QueryExecution`?)
```
val execution = context.sql(command).queryExecution
```
2. add a `isCommand` flag in `Dataset`
3. pass the `Dataset` to `SQLExecution. withNewExecutionId` and check `Dataset.isCommand`
WDYT?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812431877
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41426/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607706044
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +65,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
Review comment:
I was wondering if it's better to remove Query1 from the SQL UI, as Query2 is what `show` actually does. But I didn't come up with a good solution.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812811027
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41455/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816795743
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137136/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812519344
**[Test build #136848 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136848/testReport)** for PR 31968 at commit [`ed3b4f8`](https://github.com/apache/spark/commit/ed3b4f8c78de082a56b2f473cd025f66b6f66266).
* This patch **fails SparkR unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816677492
> e.g.: sql("drop table t").explain, the table would be dropped.
It's the same behavior before this PR, right?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-809194919
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136642/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812355321
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136837/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812313719
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r613890448
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala
##########
@@ -58,6 +58,12 @@ class QueryExecution(
val id: Long = QueryExecution.nextExecutionId
+ private var _commandExecutionIdGenerated = false
Review comment:
Let's try our best to avoid mutable states. It will make the system harder to reason about.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-813824902
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41507/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r603168384
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +64,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
+ val logicalPlan = context.sql(command).logicalPlan
Review comment:
This seems like a different issue, as the reported bug is the SQL execution appears twice, not 3 times.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-809147303
**[Test build #136642 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136642/testReport)** for PR 31968 at commit [`5159901`](https://github.com/apache/spark/commit/51599011996a49a2108ecc0737ef0667e9a3e05e).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-813959313
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136930/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819417508
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41920/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816676198
**[Test build #137136 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137136/testReport)** for PR 31968 at commit [`6d60b97`](https://github.com/apache/spark/commit/6d60b97c3e97fbc6e362aa89ea8503db5e8ecfa4).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816546253
**[Test build #137125 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137125/testReport)** for PR 31968 at commit [`4634aee`](https://github.com/apache/spark/commit/4634aee68794ef6e5229fee24ec8b7ef92ca5e52).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r603123418
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
##########
@@ -223,11 +224,18 @@ class Dataset[T] private[sql](
@transient private[sql] val logicalPlan: LogicalPlan = {
// For various commands (like DDL) and queries with side effects, we force query execution
// to happen right away to let these side effects take place eagerly.
+ def eagerRun(plan: LogicalPlan): LogicalPlan = {
+ val relation =
+ LocalRelation(plan.output, withAction("command", queryExecution)(_.executeCollect()))
Review comment:
Yes.
IIUC, For showing SQL queries and associated jobs in Spark UI, only tracking the first SQL execution is enough.
But in general, I think it's better to track all the execution ids and let the event listeners decide the behavior for the different events for one SQL query.
So another option may be that we still track and send all the SQL execution-related events to listeners, and handle the first event of a SQL query in UI listeners.
WDYT?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r604687761
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
##########
@@ -223,11 +224,18 @@ class Dataset[T] private[sql](
@transient private[sql] val logicalPlan: LogicalPlan = {
// For various commands (like DDL) and queries with side effects, we force query execution
// to happen right away to let these side effects take place eagerly.
+ def eagerRun(plan: LogicalPlan): LogicalPlan = {
+ val relation =
+ LocalRelation(plan.output, withAction("command", queryExecution)(_.executeCollect()))
+ relation.setTagValue(Dataset.DATASET_EAGER_RUN_TAG, true)
+ relation
+ }
+
val plan = queryExecution.analyzed match {
case c: Command =>
- LocalRelation(c.output, withAction("command", queryExecution)(_.executeCollect()))
Review comment:
ah I see, then your idea looks good. One suggestion is, let's not use `TreeNodeTag` here which is unreliable. We can directly add a new `fromCommand` flag in `LocalRelation`
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816582266
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41703/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816715667
Kubernetes integration test unable to build dist.
exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41714/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607795536
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +65,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
Review comment:
`toDF()` triggers a SQL execution? Do you know how that happens?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r604647620
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
##########
@@ -223,11 +224,18 @@ class Dataset[T] private[sql](
@transient private[sql] val logicalPlan: LogicalPlan = {
// For various commands (like DDL) and queries with side effects, we force query execution
// to happen right away to let these side effects take place eagerly.
+ def eagerRun(plan: LogicalPlan): LogicalPlan = {
+ val relation =
+ LocalRelation(plan.output, withAction("command", queryExecution)(_.executeCollect()))
+ relation.setTagValue(Dataset.DATASET_EAGER_RUN_TAG, true)
+ relation
+ }
+
val plan = queryExecution.analyzed match {
case c: Command =>
- LocalRelation(c.output, withAction("command", queryExecution)(_.executeCollect()))
Review comment:
But when people run commands without action in Spark application, e.g. `sql("DROP TABLE t")`, this is the place that Spark implicitly adds a `collect` action to the DataFrame.
If we don't trigger a SQL execution here, we should still add an action like `collect` somewhere in Spark in this situation, do you have any suggestions？
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-809338733
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41224/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-808110226
Gentle ping @cloud-fan, could you help to review this when having time, thanks!
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r603123418
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
##########
@@ -223,11 +224,18 @@ class Dataset[T] private[sql](
@transient private[sql] val logicalPlan: LogicalPlan = {
// For various commands (like DDL) and queries with side effects, we force query execution
// to happen right away to let these side effects take place eagerly.
+ def eagerRun(plan: LogicalPlan): LogicalPlan = {
+ val relation =
+ LocalRelation(plan.output, withAction("command", queryExecution)(_.executeCollect()))
Review comment:
Yes.
IIUC, for showing SQL queries and associated jobs in Spark UI, only tracking the first SQL execution is enough.
But in general, I think it's better to track all the execution ids and let the event listeners decide the behavior for the different events for one SQL query.
So another option may be that we still track and send all the SQL execution-related events to listeners, and handle the first event of a SQL query in UI listeners.
WDYT?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] closed pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #31968:
URL: https://github.com/apache/spark/pull/31968
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816621822
The current approach would execute command eagerly when trying to explain the command.
e.g.: `sql("drop table t").explain`, the table would be dropped. I'll try to improve this.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819268570
**[Test build #137332 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137332/testReport)** for PR 31968 at commit [`c8364b6`](https://github.com/apache/spark/commit/c8364b64725d92087af758ae086169c16c284939).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607128286
##########
File path: sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriverSuite.scala
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.hive.thriftserver
+
+import scala.collection.mutable.ListBuffer
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.scheduler.{SparkListener, SparkListenerEvent}
+import org.apache.spark.sql.execution.ui.SparkListenerSQLExecutionStart
+
+class SparkSQLDriverSuite extends SparkFunSuite {
+ private var driver: SparkSQLDriver = _
+
+ override protected def beforeEach(): Unit = {
+ System.setProperty("spark.master", "local")
+ SparkSQLEnv.init()
+ driver = new SparkSQLDriver()
+ }
+
+ override protected def afterEach(): Unit = {
+ driver.destroy()
+ driver.close()
+ SparkSQLEnv.stop()
+ }
+
+ test("Avoid wrapped in withNewExecutionId twice when run SQL with side effects") {
+ val listener = new TestListener
+ driver.context.sparkContext.addSparkListener(listener)
+ val sqls = Seq(
+ "CREATE TABLE t AS SELECT 1 AS i",
+ "SELECT * FROM t",
+ "DROP TABLE t")
+ sqls.foreach(driver.run)
+
+ assert(sqls.size === listener.sqlStartEvents.size)
+ sqls.zip(listener.sqlStartEvents).foreach { case (sql, event) =>
Review comment:
shall we make sure the length is the same before calling `zip`?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819450662
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137332/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817612548
**[Test build #137205 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137205/testReport)** for PR 31968 at commit [`d1d0def`](https://github.com/apache/spark/commit/d1d0deff251910c45e6a2f40db6b77f219afc866).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812805687
**[Test build #136878 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136878/testReport)** for PR 31968 at commit [`4e61f11`](https://github.com/apache/spark/commit/4e61f1145386696e07019e78d99f4eb02a8461f6).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817661411
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607461493
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LocalRelation.scala
##########
@@ -49,11 +49,13 @@ object LocalRelation {
*
* @param data The local collection holding the data. It doesn't need to be sent to executors
* and then doesn't need to be serializable.
+ * @param fromCommand Indicates whether the relation data is result of [[Command]]
+ * @param isStreaming Indicates whether this relation has data from a streaming source
*/
case class LocalRelation(
output: Seq[Attribute],
data: Seq[InternalRow] = Nil,
- // Indicates whether this relation has data from a streaming source.
+ fromCommand: Boolean = false,
Review comment:
Ok, will fix it.
I thought we should put the override members at the end.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812805687
**[Test build #136878 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136878/testReport)** for PR 31968 at commit [`4e61f11`](https://github.com/apache/spark/commit/4e61f1145386696e07019e78d99f4eb02a8461f6).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r603155308
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +64,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
+ val logicalPlan = context.sql(command).logicalPlan
Review comment:
Id 1 seems useless
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819300614
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41912/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] github-actions[bot] commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-886126432
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!
--
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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819296858
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816582266
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41703/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812836621
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136878/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-807969926
Can one of the admins verify this patch?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-809145650
ok to test
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r603071304
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
##########
@@ -223,11 +224,18 @@ class Dataset[T] private[sql](
@transient private[sql] val logicalPlan: LogicalPlan = {
// For various commands (like DDL) and queries with side effects, we force query execution
// to happen right away to let these side effects take place eagerly.
+ def eagerRun(plan: LogicalPlan): LogicalPlan = {
+ val relation =
+ LocalRelation(plan.output, withAction("command", queryExecution)(_.executeCollect()))
Review comment:
The problem is we do submit two Spark jobs (invoking the command and collecting the `LocalRelation`), and it's unclear which one we should wrap with a SQL execution.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607127234
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
##########
@@ -223,11 +223,16 @@ class Dataset[T] private[sql](
@transient private[sql] val logicalPlan: LogicalPlan = {
// For various commands (like DDL) and queries with side effects, we force query execution
// to happen right away to let these side effects take place eagerly.
+ def eagerRun(plan: LogicalPlan): LogicalPlan = {
Review comment:
nit: `runCommand`
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r604647620
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
##########
@@ -223,11 +224,18 @@ class Dataset[T] private[sql](
@transient private[sql] val logicalPlan: LogicalPlan = {
// For various commands (like DDL) and queries with side effects, we force query execution
// to happen right away to let these side effects take place eagerly.
+ def eagerRun(plan: LogicalPlan): LogicalPlan = {
+ val relation =
+ LocalRelation(plan.output, withAction("command", queryExecution)(_.executeCollect()))
+ relation.setTagValue(Dataset.DATASET_EAGER_RUN_TAG, true)
+ relation
+ }
+
val plan = queryExecution.analyzed match {
case c: Command =>
- LocalRelation(c.output, withAction("command", queryExecution)(_.executeCollect()))
Review comment:
But when people run commands without action in Spark application, e.g. `sql("DROP TABLE t")`, this is the place that Spark implicitly adds a `collect` action to the DataFrame.
We should still add an action like `collect` somewhere in Spark in this situation, do you have any suggestions？
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-813808206
**[Test build #136930 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136930/testReport)** for PR 31968 at commit [`c89e32d`](https://github.com/apache/spark/commit/c89e32dafe8d826051d28690e546bdbd00c5e622).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r603073246
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +64,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
+ val logicalPlan = context.sql(command).logicalPlan
Review comment:
So this is only a problem for thriftserver.
For normal spark application, people just do `sql("DROP TABLE t")` instead of `sql("DROP TABLE t").collect()`. I think we should do the same here: if `context.sql(command).qe.logicalPlan` is a `Command`, do not call `hiveResultString`.
cc @yaooqinn
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817492576
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137193/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r603105395
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +64,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
+ val logicalPlan = context.sql(command).logicalPlan
Review comment:
I guess this is not a problem only for thriftserver
```
spark.sql("show tables").show
```
is normal, but produces 3 execution ids
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812301835
**[Test build #136837 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136837/testReport)** for PR 31968 at commit [`37d64d5`](https://github.com/apache/spark/commit/37d64d53a8a59de3617b1a8114cd28d25f30c900).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812355321
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136837/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607706044
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +65,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
Review comment:
I was wondering if it's better to remove Query1 from the SQL UI, as Query2 is what `show` actually does. But I didn't come up with a good solution yet.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r610476090
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala
##########
@@ -71,7 +72,26 @@ class QueryExecution(
lazy val analyzed: LogicalPlan = executePhase(QueryPlanningTracker.ANALYSIS) {
// We can't clone `logical` here, which will reset the `_analyzed` flag.
- sparkSession.sessionState.analyzer.executeAndCheck(logical, tracker)
+ val plan = sparkSession.sessionState.analyzer.executeAndCheck(logical, tracker)
+
+ // For various commands (like DDL) and queries with side effects, we force query execution
+ // to happen right away to let these side effects take place eagerly.
+ def runCommand(plan: LogicalPlan): LogicalPlan = {
+ val qe = new QueryExecution(sparkSession, plan, isCommand = true)
+ LocalRelation(plan.output,
+ SQLExecution.withNewExecutionId(qe)(qe.executedPlan.executeCollect()),
+ fromCommand = true)
+ }
+
+ if (isCommand) {
+ plan
+ } else {
+ plan.transformUp {
Review comment:
the previous logic only handles top-level Command and Union of Command, why do we change to `transformUp` here?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819433026
**[Test build #137332 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137332/testReport)** for PR 31968 at commit [`c8364b6`](https://github.com/apache/spark/commit/c8364b64725d92087af758ae086169c16c284939).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r610512251
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala
##########
@@ -71,7 +72,26 @@ class QueryExecution(
lazy val analyzed: LogicalPlan = executePhase(QueryPlanningTracker.ANALYSIS) {
// We can't clone `logical` here, which will reset the `_analyzed` flag.
- sparkSession.sessionState.analyzer.executeAndCheck(logical, tracker)
+ val plan = sparkSession.sessionState.analyzer.executeAndCheck(logical, tracker)
+
+ // For various commands (like DDL) and queries with side effects, we force query execution
+ // to happen right away to let these side effects take place eagerly.
+ def runCommand(plan: LogicalPlan): LogicalPlan = {
+ val qe = new QueryExecution(sparkSession, plan, isCommand = true)
+ LocalRelation(plan.output,
+ SQLExecution.withNewExecutionId(qe)(qe.executedPlan.executeCollect()),
+ fromCommand = true)
+ }
+
+ if (isCommand) {
+ plan
+ } else {
+ plan.transformUp {
+ case c: Command => runCommand(c)
Review comment:
It's a flag to eager execute the command, otherwise, the logic here would end up into a dead loop and the command would never be executed.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812301835
**[Test build #136837 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136837/testReport)** for PR 31968 at commit [`37d64d5`](https://github.com/apache/spark/commit/37d64d53a8a59de3617b1a8114cd28d25f30c900).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-809340666
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41224/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r610532321
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala
##########
@@ -71,7 +72,26 @@ class QueryExecution(
lazy val analyzed: LogicalPlan = executePhase(QueryPlanningTracker.ANALYSIS) {
// We can't clone `logical` here, which will reset the `_analyzed` flag.
- sparkSession.sessionState.analyzer.executeAndCheck(logical, tracker)
+ val plan = sparkSession.sessionState.analyzer.executeAndCheck(logical, tracker)
+
+ // For various commands (like DDL) and queries with side effects, we force query execution
+ // to happen right away to let these side effects take place eagerly.
+ def runCommand(plan: LogicalPlan): LogicalPlan = {
+ val qe = new QueryExecution(sparkSession, plan, isCommand = true)
+ LocalRelation(plan.output,
+ SQLExecution.withNewExecutionId(qe)(qe.executedPlan.executeCollect()),
+ fromCommand = true)
+ }
+
+ if (isCommand) {
+ plan
+ } else {
+ plan.transformUp {
Review comment:
oops... should follow the previous behavior, will fix.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819414733
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607652899
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +65,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
Review comment:
Ok, let me try.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817465789
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812809468
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819382965
**[Test build #137341 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137341/testReport)** for PR 31968 at commit [`50b7764`](https://github.com/apache/spark/commit/50b7764bef8c48466b426cae1e57534e4fef143c).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-813824863
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817693801
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137205/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817693571
**[Test build #137205 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137205/testReport)** for PR 31968 at commit [`d1d0def`](https://github.com/apache/spark/commit/d1d0deff251910c45e6a2f40db6b77f219afc866).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607459192
##########
File path: sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriverSuite.scala
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.hive.thriftserver
+
+import scala.collection.mutable.ListBuffer
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.scheduler.{SparkListener, SparkListenerEvent}
+import org.apache.spark.sql.execution.ui.SparkListenerSQLExecutionStart
+
+class SparkSQLDriverSuite extends SparkFunSuite {
+ private var driver: SparkSQLDriver = _
+
+ override protected def beforeEach(): Unit = {
+ System.setProperty("spark.master", "local")
+ SparkSQLEnv.init()
+ driver = new SparkSQLDriver()
+ }
+
+ override protected def afterEach(): Unit = {
+ driver.destroy()
+ driver.close()
+ SparkSQLEnv.stop()
+ }
+
+ test("Avoid wrapped in withNewExecutionId twice when run SQL with side effects") {
+ val listener = new TestListener
+ driver.context.sparkContext.addSparkListener(listener)
+ val sqls = Seq(
+ "CREATE TABLE t AS SELECT 1 AS i",
+ "SELECT * FROM t",
+ "DROP TABLE t")
+ sqls.foreach(driver.run)
+
+ assert(sqls.size === listener.sqlStartEvents.size)
+ sqls.zip(listener.sqlStartEvents).foreach { case (sql, event) =>
Review comment:
The length is checked here: `assert(sqls.size === listener.sqlStartEvents.size)`
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-813824902
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41507/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817454117
**[Test build #137193 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137193/testReport)** for PR 31968 at commit [`6fcdeea`](https://github.com/apache/spark/commit/6fcdeea0d3350c6247eb481ae29481a98f61777e).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-813934815
**[Test build #136930 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136930/testReport)** for PR 31968 at commit [`c89e32d`](https://github.com/apache/spark/commit/c89e32dafe8d826051d28690e546bdbd00c5e622).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-809340666
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41224/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607841363
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +65,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
Review comment:
And I think it's better if `sql("show tables").show` only triggers one SQL execution. Query 2 should also be removed from the UI.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607650297
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +65,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
Review comment:
Can we fix the problem inside `SQLExecution.withNewExecutionId`? So that the issue can be fixed in all the places, not only SQL shell. e.g. `sql("show tables").show` and also thrift-server.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812431845
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817661467
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41785/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817454117
**[Test build #137193 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137193/testReport)** for PR 31968 at commit [`6fcdeea`](https://github.com/apache/spark/commit/6fcdeea0d3350c6247eb481ae29481a98f61777e).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819568691
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137341/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-807969926
Can one of the admins verify this patch?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819568691
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137341/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812520105
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136848/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812396717
**[Test build #136848 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136848/testReport)** for PR 31968 at commit [`ed3b4f8`](https://github.com/apache/spark/commit/ed3b4f8c78de082a56b2f473cd025f66b6f66266).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819450662
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137332/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817450051
> It's the same behavior before this PR, right?
Yeah. But it's different when run `sql("explain drop table t")`. It's fixed in https://github.com/apache/spark/commit/6d60b97c3e97fbc6e362aa89ea8503db5e8ecfa4.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819268570
**[Test build #137332 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137332/testReport)** for PR 31968 at commit [`c8364b6`](https://github.com/apache/spark/commit/c8364b64725d92087af758ae086169c16c284939).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607837639
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +65,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
Review comment:
how about we update `toDF()` and return `this` directly if the encoder is already a `RowEncoder`? We can check `Dataset.encoder.clsTag`
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816715693
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41714/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r604700219
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
##########
@@ -223,11 +224,18 @@ class Dataset[T] private[sql](
@transient private[sql] val logicalPlan: LogicalPlan = {
// For various commands (like DDL) and queries with side effects, we force query execution
// to happen right away to let these side effects take place eagerly.
+ def eagerRun(plan: LogicalPlan): LogicalPlan = {
+ val relation =
+ LocalRelation(plan.output, withAction("command", queryExecution)(_.executeCollect()))
+ relation.setTagValue(Dataset.DATASET_EAGER_RUN_TAG, true)
+ relation
+ }
+
val plan = queryExecution.analyzed match {
case c: Command =>
- LocalRelation(c.output, withAction("command", queryExecution)(_.executeCollect()))
Review comment:
Ok.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-809332088
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41224/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r604647620
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
##########
@@ -223,11 +224,18 @@ class Dataset[T] private[sql](
@transient private[sql] val logicalPlan: LogicalPlan = {
// For various commands (like DDL) and queries with side effects, we force query execution
// to happen right away to let these side effects take place eagerly.
+ def eagerRun(plan: LogicalPlan): LogicalPlan = {
+ val relation =
+ LocalRelation(plan.output, withAction("command", queryExecution)(_.executeCollect()))
+ relation.setTagValue(Dataset.DATASET_EAGER_RUN_TAG, true)
+ relation
+ }
+
val plan = queryExecution.analyzed match {
case c: Command =>
- LocalRelation(c.output, withAction("command", queryExecution)(_.executeCollect()))
Review comment:
But when people run commands without action in Spark application, e.g. `sql("DROP TABLE t")`, this is the place that Spark implicitly adds a `collect` action to the DataFrame.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yaooqinn commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r603209245
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +64,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
+ val logicalPlan = context.sql(command).logicalPlan
Review comment:
Basically, this looks the same to me but just different from the caller-sides. If we decide to handle them separately, I suggest we just move the `SQLExecution.withNewExecutionId` into the `hiveResultString` and then skip `Command` from wrapping
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607702903
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +65,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
Review comment:
As discussed in https://github.com/apache/spark/pull/31968#discussion_r603155308
When run `sql("show tables").show`, we would see 3 SQL queries in Spark SQL UI:
Query0 `sql("show tables")`: execute the `ShowTablesCommand `
Query 1 `show`: the logical plan is the same with query 0, triggered in `Dataset.getRows` https://github.com/apache/spark/blob/3b634f66c3e4a942178a1e322ae65ce82779625d/sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala#L291
Query2 `show`: show the result of the command in `LogicalRelation`
We can check the `LogicalRelation.fromCommand` flag and don't produce `executionId` for query2 in `SQLExecution.withNewExecutionId`, any thoughts?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r607127489
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/LocalRelation.scala
##########
@@ -49,11 +49,13 @@ object LocalRelation {
*
* @param data The local collection holding the data. It doesn't need to be sent to executors
* and then doesn't need to be serializable.
+ * @param fromCommand Indicates whether the relation data is result of [[Command]]
+ * @param isStreaming Indicates whether this relation has data from a streaming source
*/
case class LocalRelation(
output: Seq[Attribute],
data: Seq[InternalRow] = Nil,
- // Indicates whether this relation has data from a streaming source.
+ fromCommand: Boolean = false,
Review comment:
it seems safer to put the new parameter at the end.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-813808206
**[Test build #136930 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136930/testReport)** for PR 31968 at commit [`c89e32d`](https://github.com/apache/spark/commit/c89e32dafe8d826051d28690e546bdbd00c5e622).
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816784429
**[Test build #137136 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137136/testReport)** for PR 31968 at commit [`6d60b97`](https://github.com/apache/spark/commit/6d60b97c3e97fbc6e362aa89ea8503db5e8ecfa4).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-816611337
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137125/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817492576
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137193/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r608490932
##########
File path: sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala
##########
@@ -64,9 +65,15 @@ private[hive] class SparkSQLDriver(val context: SQLContext = SparkSQLEnv.sqlCont
new VariableSubstitution().substitute(command)
}
context.sparkContext.setJobDescription(substitutorCommand)
- val execution = context.sessionState.executePlan(context.sql(command).logicalPlan)
- hiveResponse = SQLExecution.withNewExecutionId(execution) {
Review comment:
When run `sql("show tables").collect`, there are 2 same SQL queries in SQL UI: execute `ShowTablesCommand `. This is different from running "show tables" in spark-sql.
The reason is that:
1. run "show tables" in spark-sql
we create a DataFrame from the "show tables" SQL, its `logicalPlan` is a `LocalRelation`. Then create a `QueryExecution` from the `LocalRelation` and convert it to Hive result string.
see`SaprkSQLDriver.run`
https://github.com/apache/spark/blob/06c09a79b371c5ac3e4ebad1118ed94b460f48d1/sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLDriver.scala#L67-L70
the query plan is to collect from `LocalRelation`
2. run `sql("show tables").collect`
When call collect, the `Dataset` directly use its own `QueryExecution` , see https://github.com/apache/spark/blob/06c09a79b371c5ac3e4ebad1118ed94b460f48d1/sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala#L2983
the query plan is just to execute the `ShowTablesCommand`
So it seems that we can't solve the problem by checking the `LocalRelation.fromCommand` in `SQLExecution. withNewExecutionId `
I'm thinking another way:
1. unify the behavior in `SaprkSQLDriver.run` and `Dataset.collect`
don't create extra `QueryExecution` in `SaprkSQLDriver.run` (what's the original purpose to create a new `QueryExecution`?)
```
val execution = context.sql(command).queryExecution
```
2. add a `isCommand` flag in `Dataset`
3. pass the `Dataset` to `SQLExecution. withNewExecutionId` and check `Dataset.isCommand`
WDYT?
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817693801
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137205/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817469235
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41772/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812811027
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41455/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-813959313
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136930/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-817661467
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41785/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812836621
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136878/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] wangshuo128 commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
wangshuo128 commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-809262687
> This seems like a different issue, as the reported bug is the SQL execution appears twice, not 3 times.
Run SQL below in spark-sql
```
spark-sql> DROP TABLE t;
```
Wen can see 2 SQL queries:
I think it's better to show only one SQL query in SQL UI, we have two choices:
1. only show the `DropTableCommand`
2. combine the two executions for one SQL into one item in SQL UI.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31968:
URL: https://github.com/apache/spark/pull/31968#discussion_r603434178
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/Dataset.scala
##########
@@ -223,11 +224,18 @@ class Dataset[T] private[sql](
@transient private[sql] val logicalPlan: LogicalPlan = {
// For various commands (like DDL) and queries with side effects, we force query execution
// to happen right away to let these side effects take place eagerly.
+ def eagerRun(plan: LogicalPlan): LogicalPlan = {
+ val relation =
+ LocalRelation(plan.output, withAction("command", queryExecution)(_.executeCollect()))
+ relation.setTagValue(Dataset.DATASET_EAGER_RUN_TAG, true)
+ relation
+ }
+
val plan = queryExecution.analyzed match {
case c: Command =>
- LocalRelation(c.output, withAction("command", queryExecution)(_.executeCollect()))
Review comment:
It's a bit tricky to trigger a SQL execution in the middle of DataFrame execution. I'd prefer the other way around: we don't trigger SQL execution here. We follow the common code path and only trigger SQL execution at the end like `df.collect`.
We can add some additional info to `LocalRelation`, so that its SQL UI shows the original command.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-819417508
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41920/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #31968: [SPARK-34873][SQL] Avoid wrapped in withNewExecutionId twice when run SQL with side effects
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31968:
URL: https://github.com/apache/spark/pull/31968#issuecomment-812315217
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41415/
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org