You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by LantaoJin <gi...@git.apache.org> on 2018/09/13 11:06:19 UTC
[GitHub] spark pull request #22411: [SPARK-25421][SQL] Abstract an output path field ...
GitHub user LantaoJin opened a pull request:
https://github.com/apache/spark/pull/22411
[SPARK-25421][SQL] Abstract an output path field in trait DataWritingCommand
## What changes were proposed in this pull request?
#22353 import a metadata field in ```SparkPlanInfo``` and it could dump the input location for read. Corresponding, we need add a field in ```DataWritingCommand``` for output path.
## How was this patch tested?
Unit test
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/LantaoJin/spark SPARK-25421
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22411.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #22411
----
commit 6074e36866b715954d04acd58ed0db9cdc650eaf
Author: LantaoJin <ji...@...>
Date: 2018-09-13T11:00:47Z
Abstract an output path field in trait DataWritingCommand
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22411
Since this is a new feature, we can't just merge it like #22353 without a proper design.
Making the event logs as a structured, unified and reliable source for Spark metrics looks like a good idea. Let's write a design doc to explain what we already have in the event logs, and what is missing, and how to make it reliable, and what's the issue if we read it in real time. It's better to discuss it in the dev list and see if other people have different ideas to get Spark metrics.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22411: [SPARK-25421][SQL] Abstract an output path field ...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22411#discussion_r217584439
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlanInfo.scala ---
@@ -18,6 +18,7 @@
package org.apache.spark.sql.execution
import org.apache.spark.annotation.DeveloperApi
+import org.apache.spark.sql.execution.command.{DataWritingCommand, DataWritingCommandExec}
--- End diff --
Will remove it
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22411
Gently ping @cloud-fan @dongjoon-hyun , would you please help to review?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22411
Isn't common? I am afraid not only one InsertIntoHadoopFsRelation need to added in case statment.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22411
We can't merge new features to maintenance branches(2.4 as well), so we don't need to rush here, as this feature can only be available in the next release.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22411
Agree that. Since this field is important to us. Could I refactor it following your advice and file a discussion in another Jira?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22411
If almost implementations need to add to case statment, partten matching each implementations seems weird and easy to causes missing when adds a new implementation in future.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22411
Without a thorough design, I hesitate to change the `DataWritingCommand` interface only for event log. Do you have any more plans to improve the event log?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22411
Gently ping @dongjoon-hyun @cloud-fan
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22411: [SPARK-25421][SQL] Abstract an output path field ...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22411#discussion_r217584359
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSource.scala ---
@@ -440,7 +440,7 @@ case class DataSource(
// ordering of data.logicalPlan (partition columns are all moved after data column). This
// will be adjusted within InsertIntoHadoopFsRelation.
InsertIntoHadoopFsRelationCommand(
- outputPath = outputPath,
+ outputFsPath = outputPath,
--- End diff --
This field overwrites the outputPath in DataWritingCommand and the return type is different (Path vs Option[Path]), so I rename this.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22411
Most of the information we wanted could be analyzed out from event log except some metrics in Executor side which doesn't heartbeat to Driver, e.g RPC count with NameNode. Another case is #21221, before that we had to hack code to get the similar metrics. Event log as a structured, unified, overall, replay-able log, it offers a possibility to analysis offline, even realtime. We prefer to use it since the history UI exposes less information than user expected, further more not smart and hard to customize. We are on going on this based on event log. Thanks @cloud-fan, I suggest to add this interface to ```DataWritingCommand```. Pattern matching each implementations looks trick. It looks common, maybe it could be used in physical plan optimization in future.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22411
Gently ping @cloud-fan
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22411: [SPARK-25421][SQL] Abstract an output path field ...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22411#discussion_r217453349
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSource.scala ---
@@ -440,7 +440,7 @@ case class DataSource(
// ordering of data.logicalPlan (partition columns are all moved after data column). This
// will be adjusted within InsertIntoHadoopFsRelation.
InsertIntoHadoopFsRelationCommand(
- outputPath = outputPath,
+ outputFsPath = outputPath,
--- End diff --
Could you undo this redundant change, @LantaoJin ?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22411
@cloud-fan I refactor and remove the function outputPath in ```DataWritingCommand```. Besides the unit test you could see, in my local, I added below test in ```HiveQuerySuite.scala```:
```scala
test("SPARK-25421 DataWritingCommandExec(hive) should contains 'OutputPath' metadata") {
withTable("t") {
sql("CREATE TABLE t(col_I int)")
val f = sql("INSERT OVERWRITE TABLE t SELECT 1")
assert(SparkPlanInfo.fromSparkPlan(f.queryExecution.sparkPlan).metadata
.contains("OutputPath"))
}
}
```
But since ```HiveQuerySuite``` can not access ```SparkPlanInfo```, after test passed in my local, I remove it again.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22411: [SPARK-25421][SQL] Abstract an output path field ...
Posted by dongjoon-hyun <gi...@git.apache.org>.
Github user dongjoon-hyun commented on a diff in the pull request:
https://github.com/apache/spark/pull/22411#discussion_r217454409
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/SparkPlanInfo.scala ---
@@ -18,6 +18,7 @@
package org.apache.spark.sql.execution
import org.apache.spark.annotation.DeveloperApi
+import org.apache.spark.sql.execution.command.{DataWritingCommand, DataWritingCommandExec}
--- End diff --
Do we need `DataWritingCommand` here, too?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22411
This time it's not a regression right? I'd like to not change the interface, but explicitly catch the plan we are interested in `fromSparkPlan`, e.g.
```
case DataWritingCommandExec(i: InsertIntoHadoopFsRelation, _) =>
Map("path" -> i.outputPath)
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by LantaoJin <gi...@git.apache.org>.
Github user LantaoJin commented on the issue:
https://github.com/apache/spark/pull/22411
Using pattern matching will face a problem. ```InsertIntoHiveDirCommand```,```CreateHiveTableAsSelectCommand``` and ```InsertIntoHiveTable``` are all in spark-hive module. SparkPlanInfo could not include them.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22411: [SPARK-25421][SQL] Abstract an output path field in trai...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22411
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org