Skip to content

[SPARK-27618][SQL][FOLLOW-UP] Unnecessary access to externalCatalog#24511

Closed
gatorsmile wants to merge 2 commits intoapache:masterfrom
gatorsmile:addTestcaseSpark-27618
Closed

[SPARK-27618][SQL][FOLLOW-UP] Unnecessary access to externalCatalog#24511
gatorsmile wants to merge 2 commits intoapache:masterfrom
gatorsmile:addTestcaseSpark-27618

Conversation

@gatorsmile
Copy link
Member

What changes were proposed in this pull request?

This PR is to add test cases for ensuring that we do not have unnecessary access to externalCatalog.

In the future, we can follow these examples to improve our test coverage in this area.

How was this patch tested?

N/A

@SparkQA
Copy link

SparkQA commented May 2, 2019

Test build #105072 has finished for PR 24511 at commit f373f6a.

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

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-27618][FOLLOW-UP] Unnecessary access to externalCatalog [SPARK-27618][SQL][FOLLOW-UP] Unnecessary access to externalCatalog May 2, 2019
val catalog = new SessionCatalog(externCatalog, FunctionRegistry.builtin, conf)
catalog.createDatabase(
CatalogDatabase("default", "", new URI("loc"), Map.empty),
ignoreIfExists = false)
Copy link
Member

Choose a reason for hiding this comment

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

The above three line is required to prevent NoSuchDatabaseException in the previous code before 181d190.

val func =
Alias(UnresolvedFunction("sum", Seq(UnresolvedAttribute("a")), isDistinct = false), "s")()
val plan = Project(Seq(func), testRelation)
analyzer.execute(plan)
Copy link
Member

Choose a reason for hiding this comment

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

This is another new test coverage beyond 181d190.

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. Merged to master.

It's great to have this test coverage and this example. Thanks, @gatorsmile .

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