Skip to content

[SPARK-37501][SQL] CREATE/REPLACE TABLE should qualify location for v2 command #34758

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Peng-Lei wants to merge 1 commit into from
Closed

[SPARK-37501][SQL] CREATE/REPLACE TABLE should qualify location for v2 command #34758

Peng-Lei wants to merge 1 commit into from

Conversation

@Peng-Lei
Copy link
Contributor

@Peng-Lei Peng-Lei commented Nov 30, 2021

What changes were proposed in this pull request?

  1. Rename method name makeQualifiedNamespacePath -> makeQualifiedLocationPath in CatalogUtils, so it not only for db/namespace, also for table.
  2. Override the method makeQualifiedLocationPath to take more types of parameters
  3. In CreateTableExec add handle the location properties convert.
  4. Add handle for Replace table command.

Why are the changes needed?

keep consistent for v1 and v2, and disscuss at #comments

Does this PR introduce any user-facing change?

No

How was this patch tested?

existed test case.

@github-actions github-actions bot added the SQL label Nov 30, 2021
@Peng-Lei Peng-Lei mentioned this pull request Nov 30, 2021
Closed
@SparkQA
Copy link

SparkQA commented Nov 30, 2021

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

@SparkQA
Copy link

SparkQA commented Nov 30, 2021

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

@SparkQA
Copy link

SparkQA commented Nov 30, 2021

Test build #145766 has finished for PR 34758 at commit c912456.

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

@Peng-Lei
Copy link
Contributor Author

Peng-Lei commented Dec 1, 2021

@cloud-fan @imback82 @huaxingao Could you take a look? Thank you.

LuciferYang
LuciferYang reviewed Dec 1, 2021
View reviewed changes
...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala Outdated
Copy link
Contributor

@LuciferYang LuciferYang Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

should we use a more generic method name and merge makeQualifiedNamespacePath and makeQualifiedTablePath to one

Copy link
Contributor Author

@Peng-Lei Peng-Lei Dec 1, 2021

Choose a reason for hiding this comment

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

Thank you for your advice. It would be better if merge them to one. How about makeQualifiedDBObjectPath?

Copy link
Contributor

@LuciferYang LuciferYang Dec 1, 2021

Choose a reason for hiding this comment

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

I think makeQualifiedDBObjectPath is OK

cloud-fan
cloud-fan reviewed Dec 1, 2021
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateTableExec.scala Outdated
Copy link
Contributor

@cloud-fan cloud-fan Dec 1, 2021

Choose a reason for hiding this comment

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

Can we be consistent and always qualify the path in DataSourceV2Strategy?

Copy link
Contributor Author

@Peng-Lei Peng-Lei Dec 1, 2021

Choose a reason for hiding this comment

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

en. I do it.

cloud-fan
cloud-fan reviewed Dec 1, 2021
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala Outdated
Copy link
Contributor

@cloud-fan cloud-fan Dec 1, 2021

Choose a reason for hiding this comment

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

What's the real value?

Copy link
Contributor Author

@Peng-Lei Peng-Lei Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

"file:" + WAREHOUSE_PATH + "/foo". eg:
file:/D:/code/bigdata/spark/spark-warehouse/org.apache.spark.sql.connector.DataSourceV2SQLSuite/foo

Copy link
Contributor

@cloud-fan cloud-fan Dec 1, 2021

Choose a reason for hiding this comment

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

then how about we check path.startsWith("file:") && path.endsWith("foo")?

Copy link
Contributor Author

@Peng-Lei Peng-Lei Dec 1, 2021

Choose a reason for hiding this comment

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

OK.

@SparkQA
Copy link

SparkQA commented Dec 1, 2021

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

@SparkQA
Copy link

SparkQA commented Dec 1, 2021

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

cloud-fan
cloud-fan reviewed Dec 1, 2021
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/ExternalCatalogUtils.scala Outdated
Copy link
Contributor

@cloud-fan cloud-fan Dec 1, 2021

Choose a reason for hiding this comment

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

shall we rename this and the one above to makeQualifiedDBObjectPath as well?

Copy link
Contributor Author

@Peng-Lei Peng-Lei Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

ok.

Peng-Lei
Peng-Lei commented Dec 1, 2021
View reviewed changes
...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala
val propsWithOwner = CatalogV2Util.withDefaultOwnership(props)
val newProps = props.get(TableCatalog.PROP_LOCATION).map { loc =>
props + (TableCatalog.PROP_LOCATION -> makeQualifiedDBObjectPath(loc))
}.getOrElse(props)
Copy link
Contributor Author

@Peng-Lei Peng-Lei Dec 1, 2021

Choose a reason for hiding this comment

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

As #PR not merged

@SparkQA
Copy link

SparkQA commented Dec 1, 2021

Test build #145784 has finished for PR 34758 at commit e2ad849.

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

@SparkQA
Copy link

SparkQA commented Dec 1, 2021

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

@SparkQA
Copy link

SparkQA commented Dec 1, 2021

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

@SparkQA
Copy link

SparkQA commented Dec 1, 2021

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

@SparkQA
Copy link

SparkQA commented Dec 1, 2021

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

@SparkQA
Copy link

SparkQA commented Dec 1, 2021

Test build #145802 has finished for PR 34758 at commit f24a69d.

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

@SparkQA
Copy link

SparkQA commented Dec 1, 2021

Test build #145804 has finished for PR 34758 at commit 2da9ac2.

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

@cloud-fan
Copy link
Contributor

cloud-fan commented Dec 1, 2021

thanks, merging to master!

@cloud-fan cloud-fan closed this in 38115cb Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LuciferYang LuciferYang LuciferYang left review comments

@cloud-fan cloud-fan cloud-fan approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Peng-Lei @SparkQA @cloud-fan @LuciferYang