Skip to content

Conversation

@10110346
Copy link
Contributor

@10110346 10110346 commented Apr 24, 2019

What changes were proposed in this pull requesst?

If we set hive.exec.stagingdir=.test-staging\tmp,
But the staging directory is still .hive-staging on Windows OS.

Reasons for failure:
Test code:

 val path = new Path("C:\\test\\hivetable")
  println("path.toString: " + path.toString)
  println("path.toUri.getPath: " + path.toUri.getPath)

Output:

path.toString: C:/test/hivetable
path.toUri.getPath: /C:/test/hivetable

We can see that path.toUri.getPath has one more separator than path.toString, and the separator is ' / ', not ' \ '
So stagingPathName.stripPrefix(inputPathName).stripPrefix(File.separator).startsWith(".") will return false

How was this patch tested?

  1. Existed tests
  2. Manual testing on Windows OS

@SparkQA
Copy link

SparkQA commented Apr 24, 2019

Test build #104849 has finished for PR 24446 at commit 8544174.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This drops theme:

scala> new Path("hdfs://0.0.0:8020/a/b/c").toString
res1: String = hdfs://0.0.0:8020/a/b/c

scala> new Path("hdfs://0.0.0:8020/a/b/c").toUri.getPath
res2: String = /a/b/c

So, if the theme is different from the default, it looks it's going to be ignored.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for @HyukjinKwon 's comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change this codes:

val inputPathUri: URI = inputPath.toUri
val inputPathName: String = inputPathUri.getPath

to:
val inputPathName: String = inputPath.toString

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a set of tests for this method getStagingDir?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 not to use toUri, please see the following:

scala> import org.apache.hadoop.fs.Path
import org.apache.hadoop.fs.Path

scala> val f = new java.io.File("/tmp/chk %#chk")
f: java.io.File = /tmp/chk %#chk

scala> new Path(new Path(f.toURI()), "foo").toString
res0: String = file:/tmp/chk %#chk/foo

scala> new Path(new Path(f.toURI()), "foo").toUri.toString
res1: String = file:/tmp/chk%20%25%23chk/foo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, it's a common mistake for double-encoded problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a set of tests

Sorry for the late reply.
I'll try to add some unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invokePrivate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many members are undefined in SaveAsHiveFile trait, so object creation impossible for it .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why not just private[hive] def getStagingDir?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it looks better, I will change it, thanks

@SparkQA
Copy link

SparkQA commented May 8, 2019

Test build #105247 has finished for PR 24446 at commit b8aad31.

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

@gaborgsomogyi
Copy link
Contributor

Looks like relevant issue:

java.lang.IllegalStateException: Cannot create staging directory  'file:/a/b_hive_2019-05-08_01-34-08_839_7862489166644050807-1'

@SparkQA
Copy link

SparkQA commented May 8, 2019

Test build #105251 has finished for PR 24446 at commit e34ecae.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented May 8, 2019

Test build #105254 has finished for PR 24446 at commit c02e8ba.

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

Copy link
Contributor

@gaborgsomogyi gaborgsomogyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@srowen
Copy link
Member

srowen commented May 14, 2019

@HyukjinKwon does this address your comment?

@srowen
Copy link
Member

srowen commented May 17, 2019

Merged to master

@srowen srowen closed this in 9bca99b May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants