[SPARK-22405][SQL] Add new alter table and alter database related ExternalCatalogEvent #19649

jerryshao · 2017-11-03T01:53:30Z

What changes were proposed in this pull request?

We're building a data lineage tool in which we need to monitor the metadata changes in ExternalCatalog, current ExternalCatalog already provides several useful events like "CreateDatabaseEvent" for custom SparkListener to use. But still there's some event missing, like alter database event and alter table event. So here propose to and new ExternalCatalogEvent.

How was this patch tested?

Enrich the current UT and tested on local cluster.

CC @hvanhovell please let me know your comments about current proposal, thanks.

SparkQA · 2017-11-03T05:37:14Z

Test build #83375 has finished for PR 19649 at commit a3867b7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

hvanhovell · 2017-11-03T13:31:02Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/events.scala

+/**
+ * Event fired before table schema is altered.
+ */
+case class AlterTableSchemaPreEvent(database: String, name: String) extends TableEvent


Do we need a separate event for alter schema?

hvanhovell · 2017-11-03T13:33:07Z

Looks good, one small question.

cloud-fan · 2017-11-03T13:35:29Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/ExternalCatalog.scala

   */
-  def alterTableDataSchema(db: String, table: String, newDataSchema: StructType): Unit
+  final def alterTableDataSchema(db: String, table: String, newDataSchema: StructType): Unit = {
+    postToAll(AlterTableSchemaPreEvent(db, table))


AlterTableDataSchemaXXX

shall we carry the new schema?

For me I think it is not so necessary to carry the new schema, we can query the catalog by db and table to get this newly set schema.

cloud-fan · 2017-11-03T13:40:05Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/ExternalCatalog.scala

   * this becomes a no-op.
   */
-  def alterTable(tableDefinition: CatalogTable): Unit
+  final def alterTable(tableDefinition: CatalogTable): Unit = {


I'd suggest we leave it for now and watch other alterTableXXX events instead. I feel it's an overkill to have a heavy alterTable method to handling all table metadata updating, I think we will add more and more fine-grained alter table methods in the future, like alterTableStats, alterTableDataSchema, etc. and eventually this alterTable method will go away.

This depends on which level of details we wanna collect in the event. Are there any guidelines or documentation of what events spark should monitor?
Besides, partition changes are missing, I think it's necessary to monitor these changes.

@cloud-fan , since now we expose alterTable interface for other components to leverage, if we don't track this, then looks like we missed a piece of ExternalCatalogEvents. I think for now we can add this AlterTableEvent, later on if we removed this method, then we can make this event a no-op (only kept for compatibility), what do you think?

@wzhfy , I was thinking to add partition related events, but I'm not clearly sure why this whole piece is missing and is it necessary to add partition related events? If we have an agreement on such events, I'm OK to add them.

jiangxb1987 · 2017-11-03T15:12:45Z

...atalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/ExternalCatalogEventSuite.scala

      tableType = CatalogTableType.MANAGED,
      storage = storage,
      schema = new StructType().add("id", "long"))
+    val tableDefWithSparkVersion =


Where do we need this val?

Sorry this was from my original code, will update it.

jiangxb1987 · 2017-11-03T15:14:09Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala


-  override def alterTableDataSchema(
-      db: String, table: String, newDataSchema: StructType): Unit = withClient {
+  override def doAlterTableDataSchema(


Please add description of this method.

viirya · 2017-11-04T13:55:59Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/events.scala


 import org.apache.spark.scheduler.SparkListenerEvent
+import org.apache.spark.sql.catalyst.catalog.CatalogTypes.TablePartitionSpec
+import org.apache.spark.sql.types.StructType


I didn't see StructType and TablePartitionSpec are used below.

SparkQA · 2017-11-06T11:21:53Z

Test build #83476 has finished for PR 19649 at commit 8eef970.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class AlterTableDataSchemaPreEvent(database: String, name: String) extends TableEvent
case class AlterTableDataSchemaEvent(database: String, name: String) extends TableEvent

jiangxb1987 · 2017-11-06T11:41:33Z

lgtm

viirya · 2017-11-06T13:19:26Z

LGTM

cloud-fan · 2017-11-06T14:04:13Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/ExternalCatalog.scala

   * @param newDataSchema Updated data schema to be used for the table.
   */
-  def alterTableDataSchema(db: String, table: String, newDataSchema: StructType): Unit
+  final def alterTableDataSchema(db: String, table: String, newDataSchema: StructType): Unit = {


how about alterTableStats?

cloud-fan · 2017-11-06T14:05:58Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/events.scala

+/**
+ * Event fired after a table is altered.
+ */
+case class AlterTableEvent(database: String, name: String) extends TableEvent


Maybe we can have one event for all alterTableXXX, and add one more parameter to indicate which part of the table is altered.

cloud-fan · 2017-11-07T09:42:54Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/events.scala

+ * type will generally be Table.
+ */
+object AlterTableKind extends Enumeration {
+  val Table, DataSchema, Stats = Value


shall we just use string?

I'm OK to use String, but I'd prefer strong type to avoid nasty issues.

String is better for backward compatibility, but easier to make mistake. I don't have a strong preference, cc @hvanhovell @gatorsmile

SparkQA · 2017-11-07T12:44:26Z

Test build #83538 has finished for PR 19649 at commit c79c314.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class AlterTablePreEvent(
case class AlterTableEvent(

jerryshao · 2017-11-08T09:25:17Z

One question as mentioned above also, do we need to track partition related events? @cloud-fan @hvanhovell @gatorsmile

cloud-fan · 2017-11-08T11:13:38Z

not sure, but maybe do it in a new PR?

dongjoon-hyun · 2017-11-08T19:01:24Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/events.scala


+/**
+ * Event fired before a database is altered.
+ */


Hi, @jerryshao .
We are adding AlterTableEvent and AlterDatabaseEvent. Can we have a more specific PR title instead of Add more ExternalCatalogEvent?

Sure, I will update the title.

SparkQA · 2017-11-09T04:49:35Z

Test build #83617 has finished for PR 19649 at commit 6b4fcff.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2017-11-09T10:58:06Z

thanks, merging to master!

…file ## What changes were proposed in this pull request? While spark code changes, there are new events in event log: #19649 And we used to maintain a whitelist to avoid exceptions: #15663 Currently Spark history server will stop parsing on unknown events or unrecognized properties. We may still see part of the UI data. For better compatibility, we can ignore unknown events and parse through the log file. ## How was this patch tested? Unit test Author: Wang Gengliang <[email protected]> Closes #19953 from gengliangwang/ReplayListenerBus.

jerryshao added 2 commits November 3, 2017 09:59

Add more ExternalCatalogEvent

df2cab9

Fix merging conflict

a3867b7

jerryshao force-pushed the SPARK-22405 branch from 5c628be to a3867b7 Compare November 3, 2017 02:35

hvanhovell reviewed Nov 3, 2017

View reviewed changes

cloud-fan reviewed Nov 3, 2017

View reviewed changes

jiangxb1987 reviewed Nov 3, 2017

View reviewed changes

viirya reviewed Nov 4, 2017

View reviewed changes

Addess part of the comments

8eef970

cloud-fan reviewed Nov 6, 2017

View reviewed changes

Address the comments

c79c314

cloud-fan reviewed Nov 7, 2017

View reviewed changes

dongjoon-hyun reviewed Nov 8, 2017

View reviewed changes

jerryshao changed the title ~~[SPARK-22405][SQL] Add more ExternalCatalogEvent~~ [SPARK-22405][SQL] Add new alter table and alter database related ExternalCatalogEvent Nov 9, 2017

Change to String type as comment suggested

6b4fcff

asfgit closed this in 6793a3d Nov 9, 2017

gengliangwang mentioned this pull request Dec 12, 2017

[SPARK-22763][Core]SHS: Ignore unknown events and parse through the file #19953

Closed

Uh oh!

[SPARK-22405][SQL] Add new alter table and alter database related ExternalCatalogEvent #19649

[SPARK-22405][SQL] Add new alter table and alter database related ExternalCatalogEvent #19649

Uh oh!

Conversation

jerryshao commented Nov 3, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Nov 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanhovell commented Nov 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wzhfy Nov 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 6, 2017

Uh oh!

jiangxb1987 commented Nov 6, 2017

Uh oh!

viirya commented Nov 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 7, 2017

Uh oh!

jerryshao commented Nov 8, 2017

Uh oh!

cloud-fan commented Nov 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 9, 2017

Uh oh!

cloud-fan commented Nov 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

wzhfy Nov 4, 2017 •

edited

Loading