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.

Sign up for GitHub

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

Jump to bottom

[SPARK-20024] [SQL] [test-maven] SessionCatalog reset need to set the current database of ExternalCatalog #17354

Closed
wants to merge 8 commits into from
Closed

[SPARK-20024] [SQL] [test-maven] SessionCatalog reset need to set the current database of ExternalCatalog #17354

wants to merge 8 commits into from

Conversation

gatorsmile
Copy link
Member

@gatorsmile gatorsmile commented Mar 20, 2017 •
edited
Loading

What changes were proposed in this pull request?

SessionCatalog API setCurrentDatabase does not set the current database of the underlying ExternalCatalog. Thus, weird errors could come in the test suites after we call reset. We need to fix it.

So far, have not found the direct impact in the other code paths because we expect all the SessionCatalog APIs should always use the current database value we managed, unless some of code paths skip it. Thus, we fix it in the test-only function reset().

How was this patch tested?

Multiple test case failures are observed in mvn and add a test case in SessionCatalogSuite.

@gatorsmile gatorsmile changed the title [SPARK-20024] [SQL] SessionCatalog API setCurrentDatabase need to set the current database of ExternalCatalog [WIP] [SPARK-20024] [SQL] [test-maven] SessionCatalog API setCurrentDatabase need to set the current database of ExternalCatalog [WIP] Mar 20, 2017
@gatorsmile
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Mar 20, 2017

Test build #74854 has finished for PR 17354 at commit c552c78.

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

@gatorsmile
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Mar 20, 2017

Test build #74859 has started for PR 17354 at commit c552c78.

@gatorsmile
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Mar 20, 2017

Test build #74860 has finished for PR 17354 at commit c552c78.

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

@SparkQA
Copy link

SparkQA commented Mar 20, 2017

Test build #74850 has finished for PR 17354 at commit c552c78.

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

@dilipbiswal
Copy link
Contributor

@gatorsmile Hi Sean, with this fix, do we still require the other fix in OrcSourceSuite ? We know that it can't hurt.. but it helps expose problems like this ?

@gatorsmile
Copy link
Member Author

Yes, we can revert it back

@dilipbiswal
Copy link
Contributor

@gatorsmile Thanks a lot.

@gatorsmile
Copy link
Member Author

retest this please

1 similar comment
@gatorsmile
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Mar 20, 2017

Test build #74868 has finished for PR 17354 at commit c552c78.

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

@SparkQA
Copy link

SparkQA commented Mar 20, 2017

Test build #74865 has finished for PR 17354 at commit c552c78.

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

@gatorsmile gatorsmile mentioned this pull request Mar 20, 2017
Closed
@SparkQA
Copy link

SparkQA commented Mar 20, 2017

Test build #74892 has finished for PR 17354 at commit 31f1099.

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

@SparkQA
Copy link

SparkQA commented Mar 20, 2017

Test build #74902 has finished for PR 17354 at commit f23984d.

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

@gatorsmile
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Mar 20, 2017

Test build #74900 has finished for PR 17354 at commit e860fd7.

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

dongjoon-hyun
dongjoon-hyun reviewed Mar 20, 2017
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala Outdated
@@ -226,6 +226,7 @@ class SessionCatalog(
s"${globalTempViewManager.database}.viewName.")
}
requireDbExists(dbName)
externalCatalog.setCurrentDatabase(dbName)
Copy link
Member

Choose a reason for hiding this comment

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

Hi, @gatorsmile .
Apache Spark 2.1.1 should have this fix, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

So far, everything works well without the fix. It only impacts the test cases, because when we always qualify the table names with the database names.

IMO, we should backport it to Spark 2.1.1.

Copy link
Contributor

Choose a reason for hiding this comment

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

we always qualify the table names with the database names

It explains why we don't need this fix. IMO if it only impacts tests, we should fix it in tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me move it to reset()

@SparkQA
Copy link

SparkQA commented Mar 20, 2017

Test build #74904 has finished for PR 17354 at commit f23984d.

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

@gatorsmile gatorsmile changed the title [SPARK-20024] [SQL] [test-maven] SessionCatalog API setCurrentDatabase need to set the current database of ExternalCatalog [WIP] [SPARK-20024] [SQL] [test-maven] SessionCatalog API setCurrentDatabase need to set the current database of ExternalCatalog Mar 20, 2017
@gatorsmile
Copy link
Member Author

cc @cloud-fan @yhuai @windpiger

@gatorsmile gatorsmile changed the title [SPARK-20024] [SQL] [test-maven] SessionCatalog API setCurrentDatabase need to set the current database of ExternalCatalog [SPARK-20024] [SQL] [test-maven] SessionCatalog reset need to set the current database of ExternalCatalog Mar 21, 2017
@cloud-fan
Copy link
Contributor

LGTM

@SparkQA
Copy link

SparkQA commented Mar 21, 2017

Test build #74915 has finished for PR 17354 at commit d6c63b3.

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

@gatorsmile
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Mar 21, 2017

Test build #74928 has finished for PR 17354 at commit d6c63b3.

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

@gatorsmile
Copy link
Member Author

Thanks! Merging to master.

@asfgit asfgit closed this in d2dcd67 Mar 21, 2017
@windpiger
Copy link
Contributor

late LGTM~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dongjoon-hyun dongjoon-hyun dongjoon-hyun left review comments

@cloud-fan cloud-fan cloud-fan left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gatorsmile @SparkQA @dilipbiswal @cloud-fan @windpiger @dongjoon-hyun