Skip to content

Conversation

@windpiger
Copy link
Contributor

What changes were proposed in this pull request?

Currently when we create / rename a managed table, we should get the defaultTablePath for them in ExternalCatalog, then we have two defaultTablePath logic in its two subclass HiveExternalCatalog and InMemoryCatalog, additionally there is also a defaultTablePath in SessionCatalog, so till now we have three defaultTablePath in three classes.
we'd better to unify them up to SessionCatalog

To unify them, we should move some logic from ExternalCatalog to SessionCatalog, renameTable is one of this.

while limit to the simple parameters in renameTable

  def renameTable(db: String, oldName: String, newName: String): Unit

even if we move the defaultTablePath logic to SessionCatalog, we can not pass it to renameTable.

So we can add a newTablePath parameter for renameTable in ExternalCatalog

How was this patch tested?

delete some tests in ExternalCatalogSuite which already existed in SessionCatalogSuite,
and move some other tests in ExternalCatalogSuite which does not exist in SessionCatalogSuite

catalog.dropTable("db2", "unknown_table", ignoreIfNotExists = true, purge = false)
}

test("rename table") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

already existed in SessionCatalogSuite

assert(catalog.listTables("db2").toSet == Set("tblone", "tbl2"))
}

test("rename table when database/table does not exist") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

already existed in SessionCatalogSuite

}
}

test("rename table when destination table already exists") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

move to SessionCatalogSuite

assert(!exists(db.locationUri))
}

test("create/drop/rename table should create/delete/rename the directory") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

move to SessionCatalogSuite

@SparkQA
Copy link

SparkQA commented Mar 18, 2017

Test build #74784 has finished for PR 17341 at commit a2615b7.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Mar 18, 2017

Test build #74785 has finished for PR 17341 at commit 857c4c2.

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

@windpiger
Copy link
Contributor Author

cc @cloud-fan @gatorsmile

@gatorsmile
Copy link
Member

Maybe we can close this one at first and revisit it later?

@gatorsmile
Copy link
Member

We are closing it due to inactivity. please do reopen if you want to push it forward. Thanks!

@asfgit asfgit closed this in b32bd00 Jun 27, 2017
zifeif2 pushed a commit to zifeif2/spark that referenced this pull request Nov 22, 2025
## What changes were proposed in this pull request?

This PR proposes to close stale PRs, mostly the same instances with apache#18017

I believe the author in apache#14807 removed his account.

Closes apache#7075
Closes apache#8927
Closes apache#9202
Closes apache#9366
Closes apache#10861
Closes apache#11420
Closes apache#12356
Closes apache#13028
Closes apache#13506
Closes apache#14191
Closes apache#14198
Closes apache#14330
Closes apache#14807
Closes apache#15839
Closes apache#16225
Closes apache#16685
Closes apache#16692
Closes apache#16995
Closes apache#17181
Closes apache#17211
Closes apache#17235
Closes apache#17237
Closes apache#17248
Closes apache#17341
Closes apache#17708
Closes apache#17716
Closes apache#17721
Closes apache#17937

Added:
Closes apache#14739
Closes apache#17139
Closes apache#17445
Closes apache#18042
Closes apache#18359

Added:
Closes apache#16450
Closes apache#16525
Closes apache#17738

Added:
Closes apache#16458
Closes apache#16508
Closes apache#17714

Added:
Closes apache#17830
Closes apache#14742

## How was this patch tested?

N/A

Author: hyukjinkwon <[email protected]>

Closes apache#18417 from HyukjinKwon/close-stale-pr.
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