Skip to content

Conversation

@cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

This is kind of a follow-up of #14482 . As we put CatalogTable in the logical plan directly, it makes sense to let physical plans take CatalogTable directly, instead of extracting some fields of CatalogTable in planner and then construct a new CatalogTable in physical plan.

How was this patch tested?

existing tests.

@cloud-fan
Copy link
Contributor Author

cc @yhuai @gatorsmile @liancheng

@SparkQA
Copy link

SparkQA commented Aug 26, 2016

Test build #64461 has finished for PR 14823 at commit cdd900d.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class CreateDataSourceTableCommand(table: CatalogTable, ignoreIfExists: Boolean)

@SparkQA
Copy link

SparkQA commented Aug 27, 2016

Test build #64525 has finished for PR 14823 at commit 0e9b3d2.

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

@gatorsmile
Copy link
Member

I missed this ping. Will review it tomorrow.

// TODO: this may be wrong for non file-based data source like JDBC, which should be external
// even there is no `path` in options. We should consider allow the EXTERNAL keyword.
val tableType = if (new CaseInsensitiveMap(options).contains("path")) {
CatalogTableType.EXTERNAL
Copy link
Member

Choose a reason for hiding this comment

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

We can remove https://github.com/cloud-fan/spark/blob/0e9b3d2a81f0251f710eae42adca307cd9c69d33/sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala#L316-L318 ?

After this PR, it becomes much clear to users. We can support Create External Data Source Table! We just need to confirm the path is provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing this means we change the syntax, I'd like to avoid it in this PR, and that's why I leave this TODO here, we can do it in follow-up

@gatorsmile
Copy link
Member

LGTM except one minor comment.

@SparkQA
Copy link

SparkQA commented Sep 1, 2016

Test build #64759 has finished for PR 14823 at commit 00bf25b.

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

@SparkQA
Copy link

SparkQA commented Sep 1, 2016

Test build #64762 has finished for PR 14823 at commit 52a40d9.

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

@cloud-fan
Copy link
Contributor Author

thanks for the review, merging to master!

@asfgit asfgit closed this in 8e740ae Sep 1, 2016
@cloud-fan cloud-fan deleted the create-table branch December 14, 2016 12:33
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.

3 participants