[SPARK-19463][SQL]refresh cache after the InsertIntoHadoopFsRelationCommand

[windpiger]

What changes were proposed in this pull request?

If we first cache a DataSource table, then we insert some data into the table, we should refresh the data in the cache after the insert command.

How was this patch tested?

unit test added

[SparkQA]

Test build #72408 has finished for PR 16809 at commit 8aaef3f.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #72410 has finished for PR 16809 at commit bf2bc1d.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #72419 has finished for PR 16809 at commit 15350f1.

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

[windpiger]

cc @cloud-fan @gatorsmile

[cloud-fan]

This is a behavior change rather than a bug fix, but I think this new behavior makes more sense. cc @gatorsmile to confirm.

[gatorsmile]

@cloud-fan The new behavior looks reasonable to me, unless users are expecting to keey the original cached data.

I went over the change history. I found @sameeragarwal did this in #13566 on purpose, even if he reported the issue in the initial PR (#13419).

@sameeragarwal @hvanhovell @davies what is the reason we did not automatically call the refreshByPath after insert?

[gatorsmile]

Found the design doc: https://docs.google.com/document/d/1h5SzfC5UsvIrRpeLNDKSMKrKJvohkkccFlXo-GBAwQQ/edit?ts=574f717f#

An alternative is to support a new command REFRESH path that invalidates and refreshes all the cached data (and the associated metadata) for any dataframe that contains the given data source path. This acts as an explicit hammer without modifying the default behavior. Given that it’s fairly late to make significant changes in 2.0, this option might be less intrusive to the default behavior.

Should we revisit what is the expected default behavior in 2.2?

[windpiger]

thanks a lot! It seems that add a REFRESH command is to not modify the default behavior. if user want to refresh, they call the command manually.

@gatorsmile @cloud-fan @sameeragarwal @hvanhovell @davies let us rethink the default behavior? It is resonable to refresh after Insert auto, or use refresh manually?

[cloud-fan]

where do we refresh table for table insertion? will we fresh twice(table and path)?

[windpiger]

I just found refresh table related to table insertion when DataFrameWriter.saveAsTable with overwrite mode, and InsetIntoHiveTable. InsertHadoopFsRelation need to refresh table?

[cloud-fan]

This is only useful when the fileIndex is None right?

[windpiger]

fileIndex is not None also need to refresh

[cloud-fan]

why? we will do fileIndex.foreach(_.refresh()) at the end, what's the difference between this and refreshByPath?

[windpiger]

if we cache the table, refreshByPath will unpersist it
https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala#L176

[SparkQA]

Test build #73271 has finished for PR 16809 at commit 12b68a7.

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

[SparkQA]

Test build #73516 has finished for PR 16809 at commit f8ccc2f.

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

[cloud-fan]

thanks, merging to master!