[SPARK-31935][SQL][FOLLOWUP] Hadoop file system config should be effective in data source options #28948

cloud-fan · 2020-06-29T15:13:09Z

What changes were proposed in this pull request?

This is a followup of #28760 to fix the remaining issues:

should consider data source options when refreshing cache by path at the end of InsertIntoHadoopFsRelationCommand
should consider data source options when inferring schema for file source
should consider data source options when getting the qualified path in file source v2.

Why are the changes needed?

We didn't catch these issues in #28760, because the test case is to check error when initializing the file system. If we initialize the file system multiple times during a simple read/write action, the test case actually only test the first time.

Does this PR introduce any user-facing change?

No

How was this patch tested?

rewrite the test to make sure the entire data source read/write action can succeed.

cloud-fan · 2020-06-29T15:14:01Z

cc @gengliangwang @liancheng @dongjoon-hyun

dongjoon-hyun · 2020-06-29T15:16:37Z

Oh, thanks for pinging me, @cloud-fan .

dongjoon-hyun · 2020-06-29T15:26:34Z

sql/core/src/test/scala/org/apache/spark/sql/FileBasedDataSourceSuite.scala

+class FakeFileSystemRequiringDSOption extends LocalFileSystem {
+  override def initialize(name: URI, conf: Configuration): Unit = {
+    super.initialize(name, conf)
+    require(conf.get("ds_option", "") == "value")


Nice check!

dongjoon-hyun · 2020-06-29T15:38:09Z

sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala

+   * Tries to re-cache all the cache entries that contain `path` in one or more
+   * `HadoopFsRelation` node(s) as part of its logical plan.
+   */
+  def recacheByPath(spark: SparkSession, path: Path, fs: FileSystem): Unit = {


Just wondering if we can have a consistent parameter name for all recacheByPaths. Currently, resourcePath and path are used in a mixed way. Is it for distinguish Path class from String class? Even in that case, resourcePath: Path also looks reasonable. Or, we can use path for both if we can.

gengliangwang · 2020-06-29T21:45:36Z

sql/core/src/main/scala/org/apache/spark/sql/execution/CacheManager.scala

   * Tries to re-cache all the cache entries that contain `resourcePath` in one or more
   * `HadoopFsRelation` node(s) as part of its logical plan.
   */
  def recacheByPath(spark: SparkSession, resourcePath: String): Unit = {


how about we change the method as

def recacheByPath(spark: SparkSession, resourcePath: String, options: Map[String, String]=Map.empty)

so that we can avoid the new method below?

That also works, but it feels a little bit weird to couple the data source options concept with the cache manager...

liancheng · 2020-06-30T06:23:40Z

@cloud-fan, thanks for fixing this! We should probably backport this one to 2.4 as well.

cloud-fan · 2020-06-30T06:48:19Z

Yea, I'll open a new PR for 2.4 after this one gets merged.

SparkQA · 2020-06-30T16:18:34Z

Test build #124651 has started for PR 28948 at commit 2d7975e.

dongjoon-hyun · 2020-06-30T17:14:40Z

Hi, @liancheng and @cloud-fan .
@gengliangwang 's original SPARK-31935 was Improvement and landed only at branch-3.0.
To have this in branch-2.4, could you adjust the JIRA issue into Bug and add 2.4.x into Affected Versions first?

Please note that I'm not against backporting of SPARK-31935 and this follow-up in branch-2.4.

Also, cc @gatorsmile

cloud-fan · 2020-06-30T17:28:55Z

@gengliangwang Can you open a new PR to backport #28760 to 2.4?

gengliangwang · 2020-07-01T00:55:10Z

@gengliangwang Can you open a new PR to backport #28760 to 2.4?

@cloud-fan it is already back ported in #28771

gengliangwang · 2020-07-01T04:33:28Z

retest this please.

liancheng · 2020-07-01T08:39:06Z

@dongjoon-hyun, thanks for reminding. Didn't see your comment at first, but I've already updated the "Affects Version/s" field in the Jira ticket. Also saw that it's already marked as a bug.

SparkQA · 2020-07-01T09:55:41Z

Test build #124742 has finished for PR 28948 at commit 2d7975e.

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

cloud-fan · 2020-07-01T12:02:40Z

retest this please

gengliangwang

LGTM, thanks for the fix!

dongjoon-hyun · 2020-07-01T15:06:27Z

Thank you all!

SparkQA · 2020-07-01T19:29:42Z

Test build #124776 has finished for PR 28948 at commit 2d7975e.

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

gengliangwang · 2020-07-01T22:07:27Z

Thanks, merging to master/3.0

…ctive in data source options ### What changes were proposed in this pull request? This is a followup of #28760 to fix the remaining issues: 1. should consider data source options when refreshing cache by path at the end of `InsertIntoHadoopFsRelationCommand` 2. should consider data source options when inferring schema for file source 3. should consider data source options when getting the qualified path in file source v2. ### Why are the changes needed? We didn't catch these issues in #28760, because the test case is to check error when initializing the file system. If we initialize the file system multiple times during a simple read/write action, the test case actually only test the first time. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? rewrite the test to make sure the entire data source read/write action can succeed. Closes #28948 from cloud-fan/fix. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Gengliang Wang <[email protected]> (cherry picked from commit 6edb20d) Signed-off-by: Gengliang Wang <[email protected]>

gengliangwang · 2020-07-01T22:12:47Z

@cloud-fan there are a lot of conflicts when merging this one to branch-2.4. We will have to do it manually.

…ctive in data source options This is a followup of apache#28760 to fix the remaining issues: 1. should consider data source options when refreshing cache by path at the end of `InsertIntoHadoopFsRelationCommand` 2. should consider data source options when inferring schema for file source 3. should consider data source options when getting the qualified path in file source v2. We didn't catch these issues in apache#28760, because the test case is to check error when initializing the file system. If we initialize the file system multiple times during a simple read/write action, the test case actually only test the first time. No rewrite the test to make sure the entire data source read/write action can succeed. Closes apache#28948 from cloud-fan/fix. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Gengliang Wang <[email protected]> (cherry picked from commit 6edb20d) Signed-off-by: Gengliang Wang <[email protected]>

… effective in data source options ### What changes were proposed in this pull request? backport #28948 This is a followup of #28760 to fix the remaining issues: 1. should consider data source options when refreshing cache by path at the end of `InsertIntoHadoopFsRelationCommand` 2. should consider data source options when inferring schema for file source 3. should consider data source options when getting the qualified path in file source v2. ### Why are the changes needed? We didn't catch these issues in #28760, because the test case is to check error when initializing the file system. If we initialize the file system multiple times during a simple read/write action, the test case actually only test the first time. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? rewrite the test to make sure the entire data source read/write action can succeed. Closes #28973 from cloud-fan/pick. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

fix

16cae97

probot-autolabeler bot added AVRO SQL labels Jun 29, 2020

dongjoon-hyun reviewed Jun 29, 2020

View reviewed changes

gengliangwang reviewed Jun 29, 2020

View reviewed changes

resource path

2d7975e

dongjoon-hyun approved these changes Jul 1, 2020

View reviewed changes

gengliangwang approved these changes Jul 1, 2020

View reviewed changes

gengliangwang closed this in 6edb20d Jul 1, 2020

cloud-fan mentioned this pull request Jul 2, 2020

[2.4][SPARK-31935][SQL][FOLLOWUP] Hadoop file system config should be effective in data source options #28973

Closed

[SPARK-31935][SQL][FOLLOWUP] Hadoop file system config should be effective in data source options #28948

[SPARK-31935][SQL][FOLLOWUP] Hadoop file system config should be effective in data source options #28948

Uh oh!

Conversation

cloud-fan commented Jun 29, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

cloud-fan commented Jun 29, 2020

Uh oh!

dongjoon-hyun commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun Jun 29, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jun 29, 2020 • 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!

gengliangwang Jun 29, 2020

Choose a reason for hiding this comment

Uh oh!

liancheng Jun 30, 2020

Choose a reason for hiding this comment

Uh oh!

liancheng commented Jun 30, 2020

Uh oh!

cloud-fan commented Jun 30, 2020

Uh oh!

SparkQA commented Jun 30, 2020

Uh oh!

dongjoon-hyun commented Jun 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Jun 30, 2020

Uh oh!

gengliangwang commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gengliangwang commented Jul 1, 2020

Uh oh!

liancheng commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jul 1, 2020

Uh oh!

cloud-fan commented Jul 1, 2020

Uh oh!

gengliangwang left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 1, 2020

Uh oh!

SparkQA commented Jul 1, 2020

Uh oh!

gengliangwang commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gengliangwang commented Jul 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dongjoon-hyun commented Jun 29, 2020 •

edited

Loading

dongjoon-hyun Jun 29, 2020 •

edited

Loading

dongjoon-hyun commented Jun 30, 2020 •

edited

Loading

gengliangwang commented Jul 1, 2020 •

edited

Loading

liancheng commented Jul 1, 2020 •

edited

Loading

gengliangwang commented Jul 1, 2020 •

edited

Loading