Skip to content

Conversation

@squito
Copy link
Contributor

@squito squito commented Oct 15, 2018

What changes were proposed in this pull request?

LOAD DATA INPATH didn't work if the defaultFS included a port for hdfs.
Handling this just requires a small change to use the correct URI
constructor.

How was this patch tested?

Added a unit test, ran all tests via jenkins

LOAD DATA INPATH didn't work if the defaultFS included a port for hdfs.
Handling this just requires a small change to use the correct URI
constructor.
Copy link
Contributor Author

@squito squito left a comment

Choose a reason for hiding this comment

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

I wasn't sure about the best place to put the test, I decided to put it next to the other tests on LOAD DATA INPATH in SQLQuerySuites, though its not running full sql queries. I thought a test just on makeQualified would be much easier to deal with than a full query. Is there a standard place to put tests on helper functions for files like tables.scala?

also if a reviewer would just rewrite themselves to organize tests in the right way, that is fine, this can be abandoned.

}
try {
val newUri = new URI(scheme, authority, pathUri.getPath, pathUri.getFragment)
val newUri = new URI(scheme, authority, pathUri.getPath, null, pathUri.getFragment)
Copy link
Contributor Author

@squito squito Oct 15, 2018

Choose a reason for hiding this comment

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

this change is all that's needed to fix the bug, the rest is for testing

@srowen
Copy link
Member

srowen commented Oct 15, 2018

CC @sujith71955

@vanzin
Copy link
Contributor

vanzin commented Oct 15, 2018

LGTM.

@cloud-fan
Copy link
Contributor

LGTM

@SparkQA
Copy link

SparkQA commented Oct 16, 2018

Test build #97413 has finished for PR 22733 at commit fe17691.

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

@vanzin
Copy link
Contributor

vanzin commented Oct 16, 2018

Merging to master / 2.4

asfgit pushed a commit that referenced this pull request Oct 16, 2018
## What changes were proposed in this pull request?

LOAD DATA INPATH didn't work if the defaultFS included a port for hdfs.
Handling this just requires a small change to use the correct URI
constructor.

## How was this patch tested?

Added a unit test, ran all tests via jenkins

Closes #22733 from squito/SPARK-25738.

Authored-by: Imran Rashid <irashid@cloudera.com>
Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>
(cherry picked from commit fdaa998)
Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>
@asfgit asfgit closed this in fdaa998 Oct 16, 2018
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
## What changes were proposed in this pull request?

LOAD DATA INPATH didn't work if the defaultFS included a port for hdfs.
Handling this just requires a small change to use the correct URI
constructor.

## How was this patch tested?

Added a unit test, ran all tests via jenkins

Closes apache#22733 from squito/SPARK-25738.

Authored-by: Imran Rashid <irashid@cloudera.com>
Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>
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.

5 participants