Skip to content

Conversation

@ulysses-you
Copy link
Contributor

What changes were proposed in this pull request?

Change URI back to URL.

The issue JIRA SPARK-32240.
Some history #14008 and #14488.

Why are the changes needed?

select parse_url('https://a.b.c/index.php?params1=a|b&params2=x', 'HOST')

-- before this PR
null

-- after this PR
a.b.c

Hive also use URL to parse url string.

Does this PR introduce any user-facing change?

Yes, correct the result.

How was this patch tested?

Add test.

@github-actions github-actions bot added the SQL label Nov 11, 2020
@SparkQA
Copy link

SparkQA commented Nov 11, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35538/

@SparkQA
Copy link

SparkQA commented Nov 11, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35538/

@srowen
Copy link
Member

srowen commented Nov 11, 2020

As in the JIRA, that's not quite a valid http URI. I realize the issue is Hive compatibility, but, changing this behavior may break code in other ways. I'd rather do the right-er thing here all else equal and leave it.

@SparkQA
Copy link

SparkQA commented Nov 11, 2020

Test build #130932 has finished for PR 30333 at commit f62e88b.

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

@ulysses-you
Copy link
Contributor Author

@srowen Yeah, that's a Hive compatible issue. I always want to eliminate such issue in first thought because of our case. But I agree with your point that it's not a valid url, seems we can close both PR and JIRA.

@ulysses-you
Copy link
Contributor Author

BTW, do we need throw exception in ANSI mode if we accept a no-vaild url instead of return null ? cc @srowen @cloud-fan

@cloud-fan
Copy link
Contributor

I think so. @ulysses-you can you create a sub-task in https://issues.apache.org/jira/browse/SPARK-33275? thanks!

@ulysses-you
Copy link
Contributor Author

OK, will do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants