Skip to content

Conversation

@cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

This is a followup of #30267

Inspired by #30886, it's better to have 2 methods def dropTable and def purgeTable, than def dropTable(ident) and def dropTable(ident, purge).

Why are the changes needed?

  1. make the APIs orthogonal. Previously, def dropTable(ident, purge) calls def dropTable(ident) and is a superset.
  2. simplifies the catalog implementation a little bit. Now the if (purge) ... else ... check is done at the Spark side.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

existing tests

@cloud-fan
Copy link
Contributor Author

cc @imback82 @rdblue @MaxGekk

@github-actions github-actions bot added the SQL label Dec 22, 2020
* @return true if a table was deleted, false if no table exists for the identifier
* @throws UnsupportedOperationException If table purging is not supported
*
* @since 3.1.0
Copy link
Member

Choose a reason for hiding this comment

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

Are you going to backport this to branch-3.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, otherwise it's a breaking change and is not accepted.

@MaxGekk
Copy link
Member

MaxGekk commented Dec 22, 2020

There are tests for the dropTable() method in TableCatalogSuite:

test("dropTable") {
val catalog = newCatalog()
assert(!catalog.tableExists(testIdent))
catalog.createTable(testIdent, schema, Array.empty, emptyProps)
assert(catalog.tableExists(testIdent))
val wasDropped = catalog.dropTable(testIdent)
assert(wasDropped)
assert(!catalog.tableExists(testIdent))
}

Should we add similar primitive tests for purgeTable() there?

Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

LGTM

@SparkQA
Copy link

SparkQA commented Dec 22, 2020

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

@SparkQA
Copy link

SparkQA commented Dec 22, 2020

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

Copy link
Contributor

@imback82 imback82 left a comment

Choose a reason for hiding this comment

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

LGTM

@SparkQA
Copy link

SparkQA commented Dec 22, 2020

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

@SparkQA
Copy link

SparkQA commented Dec 22, 2020

Test build #133223 has finished for PR 30890 at commit 879ea6d.

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

@SparkQA
Copy link

SparkQA commented Dec 22, 2020

Test build #133228 has finished for PR 30890 at commit 3b57205.

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

Copy link
Member

@HyukjinKwon HyukjinKwon left a comment

Choose a reason for hiding this comment

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

LGTM

@HyukjinKwon
Copy link
Member

Merged to master and branch-3.1.

HyukjinKwon pushed a commit that referenced this pull request Dec 23, 2020
This is a followup of #30267

Inspired by #30886, it's better to have 2 methods `def dropTable` and `def purgeTable`, than `def dropTable(ident)` and `def dropTable(ident, purge)`.

1. make the APIs orthogonal. Previously, `def dropTable(ident, purge)` calls `def dropTable(ident)` and is a superset.
2. simplifies the catalog implementation a little bit. Now the `if (purge) ... else ...` check is done at the Spark side.

No.

existing tests

Closes #30890 from cloud-fan/purgeTable.

Authored-by: Wenchen Fan <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
(cherry picked from commit ec1560a)
Signed-off-by: HyukjinKwon <[email protected]>
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.

5 participants