[SPARK-25421][SQL] Abstract an output path field in trait DataWritingCommand

[LantaoJin]

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

[LantaoJin]

Gently ping @cloud-fan@dongjoon-hyun , would you please help to review?

[dongjoon-hyun]

Could you undo this redundant change, @LantaoJin ?

[LantaoJin]

This field overwrites the outputPath in DataWritingCommand and the return type is different (Path vs Option[Path]), so I rename this.

[dongjoon-hyun]

Do we need DataWritingCommand here, too?

[LantaoJin]

Will remove it

[LantaoJin]

Gently ping @dongjoon-hyun@cloud-fan

[cloud-fan]

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)

[LantaoJin]

Isn't common? I am afraid not only one InsertIntoHadoopFsRelation need to added in case statment.

[LantaoJin]

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.

[cloud-fan]

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?

[LantaoJin]

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.

[cloud-fan]

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.

[LantaoJin]

Agree that. Since this field is important to us. Could I refactor it following your advice and file a discussion in another Jira?

[LantaoJin]

Using pattern matching will face a problem. InsertIntoHiveDirCommand,CreateHiveTableAsSelectCommand and InsertIntoHiveTable are all in spark-hive module. SparkPlanInfo could not include them.

[cloud-fan]

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.

[LantaoJin]

@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:

 test("SPARK-25421 DataWritingCommandExec(hive) should contains 'OutputPath' metadata") {
withTable("t") {
sql("CREATE TABLE t(col_I int)")
valf= 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.

[LantaoJin]

Gently ping @cloud-fan

[AmplabJenkins]

Can one of the admins verify this patch?

[github-actions]

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!