Skip to content
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.

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

[SPARK-32156][TESTS][SQL] Refactor two similar test cases from SPARK-31061 in HiveExternalCatalogSuite #28980

Closed

Conversation

TJX2014
Copy link
Contributor

@TJX2014 TJX2014 commented Jul 2, 2020

What changes were proposed in this pull request?

1.Merge two similar tests for SPARK-31061 and make the code clean.
2.Fix table alter issue due to lose path.

Why are the changes needed?

Because this two tests for SPARK-31061 is very similar and could be merged.
And the first test case should use rawTable instead of parquetTable to alter.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit test.

val parquetTable = CatalogTable(
identifier = TableIdentifier("parq_tbl", Some("db1")),
tableType = CatalogTableType.MANAGED,
storage = storageFormat.copy(locationUri = Some(new URI("file:/some/path"))),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

file:/some/path seems not very suitable.

val rawTable = externalCatalog.getTable("db1", "parq_tbl")
assert(rawTable.provider === Some("parquet"))

val fooTable = parquetTable.copy(provider = Some("foo"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems we should use rawTableget from catalog instead of parquetTable from the beginning.

@TJX2014
Copy link
Contributor Author

TJX2014 commented Jul 2, 2020

cc @cloud-fan @brkyvz

catalog.alterTable(fooTable)
val alteredTable = externalCatalog.getTable("db1", "parq_tbl")
assert(alteredTable.provider === Some("foo"))
Seq("parquet", "hive").foreach(i => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: foreach(i => { -> foreach { provider =>

@HyukjinKwon HyukjinKwon changed the title [SPARK-32156][TEST][FOLLOWUP] SPARK-31061 has two very similar tests could merge and somewhere could be improved [SPARK-32156][TESTS][SQL] Refactor two similar test cases from SPARK-31061 in HiveExternalCatalogSuite Jul 2, 2020
@cloud-fan
Copy link
Contributor

ok to test

@SparkQA
Copy link

SparkQA commented Jul 2, 2020

Test build #124900 has finished for PR 28980 at commit 899780e.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @TJX2014 and all.
Merged to master.

@TJX2014
Copy link
Contributor Author

TJX2014 commented Jul 2, 2020

Thank you very much.

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