[SPARK-26946][SQL][FOLLOWUP] Require lookup function by jzhuge · Pull Request #24689 · apache/spark

jzhuge · 2019-05-23T16:59:36Z

What changes were proposed in this pull request?

Require the lookup function with interface LookupCatalog. Rationale is in the review comments below.

Make Analyzer abstract. BaseSessionStateBuilder and HiveSessionStateBuilder implements lookupCatalog with a call to SparkSession.catalog().

Existing test cases and those that don't need catalog lookup will use a newly added TestAnalyzer with a default lookup function that throws CatalogNotFoundException("No catalog lookup function").

Rewrote the unit test for LookupCatalog to demonstrate the interface can be used anywhere, not just Analyzer.

Removed Analyzer parameter lookupCatalog because we can override in the following manner:

new Analyzer() {
override def lookupCatalog(name: String): CatalogPlugin = ???
}

How was this patch tested?

Existing unit tests.

Treat it as catalog not found.

SparkQA · 2019-05-23T20:05:53Z

Test build #105734 has finished for PR 24689 at commit 59363b2.

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

dongjoon-hyun · 2019-05-26T16:08:11Z

Retest this please.

SparkQA · 2019-05-26T19:22:49Z

Test build #105799 has finished for PR 24689 at commit 59363b2.

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

jzhuge · 2019-05-26T22:50:01Z

Removed commit [SPARK-26946][SQL][FOLLOWUP] Undo changes to Analyzer per review in #24686

SparkQA · 2019-05-27T01:53:31Z

Test build #105806 has finished for PR 24689 at commit b62182f.

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

HyukjinKwon

Hey, @jzhuge, can we clarify what handling None means in PR description?

SparkQA · 2019-05-27T07:45:21Z

Test build #105817 has finished for PR 24689 at commit d2085e5.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class LookupCatalogSuite extends SparkFunSuite with Inside

rdblue · 2019-05-27T18:06:18Z

@jzhuge, these changes look okay, but I think the right path is to always require a lookup function and get rid of the option. There is never a time when these should be used without a catalog lookup function and not requiring that one is supplied leads to avoidable errors.

I've been testing a FunctionCatalog implementation and it was easy to forget to provide a catalog lookup. But you always need a catalog lookup that does something. You may want to default in some cases to throwing NoSuchCatalogException, but that depends on the context, not whether you forgot to supply the lookup.

jzhuge · 2019-05-28T01:01:55Z

Well said @rdblue. I will proceed with the requirement of a lookup function. Since the interface is not widely used (yet), not too many places need update.

This reverts commit d2085e5.

…t defined" This reverts commit b62182f.

(cherry picked from commit 4e99de0)

SparkQA · 2019-05-28T07:05:02Z

Test build #105857 has finished for PR 24689 at commit 0f4d9aa.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

jzhuge · 2019-05-28T15:29:20Z

Not sure what the following failure is in test build #105857:

MLlib regression algorithms, except for tree-based algorithms: Spark package found in SPARK_HOME: /home/jenkins/workspace/SparkPullRequestBuilder
........................................................[error] running /home/jenkins/workspace/SparkPullRequestBuilder/R/run-tests.sh ; process was terminated by signal 9

SparkQA · 2019-05-28T20:55:14Z

Test build #105877 has finished for PR 24689 at commit 0f73998.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
abstract class Analyzer(

rdblue · 2019-05-28T21:12:34Z

 * [[UnresolvedRelation]]s into fully typed objects using information in a [[SessionCatalog]].
 */
-class Analyzer(
+abstract class Analyzer(


Why not continue to pass a lookup function in some paths and default the analyzer to use one that throws CatalogNotFoundException? That would avoid a lot of these changes.
classAnalyzer(..., catalogLookup: String=>CatalogPlugin) { defthis(...) = { this(..., _ =>thrownewCatalogNotFoundException("Analyzer does not support multiple catalogs")) } } overridedeflookupCatalog(name: String):CatalogPlugin= catalogLookup(name)

Reverted commit Make Analyzer abstract and add TestAnalyzer to minimize changes to test classes.
If you feel strongly, I can add the 4th parameter override val lookupCatalog: String => CatalogPlugin to Analyzer constructor. Although users can override in the following manner:
new Analyzer() { override def lookupCatalog(name: String): CatalogPlugin = ??? }

Sounds good to me.

This reverts commit 0f73998.

SparkQA · 2019-05-29T03:16:51Z

Test build #105884 has finished for PR 24689 at commit cecbea0.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class Analyzer(

rdblue · 2019-05-29T18:27:49Z

Looks good to me. One minor comment: it doesn't look like Analyzer implementations created in BaseSessionStateBuilder actually override the lookup method to call the session's catalog lookup. I would probably include that in this PR, but we can also add it in later PRs when it is used.

+1, I think this is ready to merge either way.

@cloud-fan, @dongjoon-hyun, @HyukjinKwon, could you take a look at this DSv2 PR? Thanks!

cloud-fan · 2019-05-30T01:22:53Z

LGTM, merging to master!

jzhuge · 2019-05-30T01:40:07Z

Thanks @HyukjinKwon @rdblue @cloud-fan !

## What changes were proposed in this pull request? Require the lookup function with interface LookupCatalog. Rationale is in the review comments below. Make `Analyzer` abstract. BaseSessionStateBuilder and HiveSessionStateBuilder implements lookupCatalog with a call to SparkSession.catalog(). Existing test cases and those that don't need catalog lookup will use a newly added `TestAnalyzer` with a default lookup function that throws` CatalogNotFoundException("No catalog lookup function")`. Rewrote the unit test for LookupCatalog to demonstrate the interface can be used anywhere, not just Analyzer. Removed Analyzer parameter `lookupCatalog` because we can override in the following manner: ``` new Analyzer() { override def lookupCatalog(name: String): CatalogPlugin = ??? } ``` ## How was this patch tested? Existing unit tests. Closesapache#24689 from jzhuge/SPARK-26946-follow. Authored-by: John Zhuge <jzhuge@apache.org> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

[SPARK-26946][SQL][FOLLOWUP] Handle lookupCatalog function not defined
b62182f
Treat it as catalog not found.

jzhuge mentioned this pull request May 23, 2019
[SPARK-27813][SQL] DataSourceV2: Add DropTable logical operation #24686
Closed

jzhuge force-pushed the SPARK-26946-follow branch from 59363b2 to b62182fCompare May 26, 2019 22:47

HyukjinKwon reviewed May 27, 2019
View reviewed changes

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalog/v2/LookupCatalog.scala Outdated

HyukjinKwon reviewed May 27, 2019
View reviewed changes

Add unit test
d2085e5

jzhuge mentioned this pull request May 27, 2019
[SPARK-27322][SQL] DataSourceV2: Select from multiple catalogs #24718
Closed

rdblue reviewed May 27, 2019
View reviewed changes

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalog/v2/LookupCatalog.scala Outdated

rdblue reviewed May 27, 2019
View reviewed changes

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalog/v2/LookupCatalog.scala Outdated

jzhuge added 3 commits May 27, 2019 21:12

Revert "Add unit test"
d5564a8
This reverts commit d2085e5.

Revert "[SPARK-26946][SQL][FOLLOWUP] Handle lookupCatalog function no…
bb4514c
…t defined" This reverts commit b62182f.

[SPARK-26946][SQL][FOLLOWUP] Require lookup function
0f4d9aa
(cherry picked from commit 4e99de0)

jzhuge changed the title ~~[SPARK-26946][SQL][FOLLOWUP] Handle lookupCatalog function not defined~~[SPARK-26946][SQL][FOLLOWUP] Require lookup functionMay 28, 2019

Make Analyzer abstract and add TestAnalyzer
0f73998

rdblue reviewed May 28, 2019
View reviewed changes

Revert "Make Analyzer abstract and add TestAnalyzer"
cecbea0
This reverts commit 0f73998.

cloud-fan closed this in 953b8e8May 30, 2019

rdblue mentioned this pull request Jun 6, 2019
[SPARK-27965][SQL] Add extractors for v2 catalog transforms. #24812
Closed

Uh oh!

Conversation

jzhuge commented May 23, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented May 23, 2019

Uh oh!

dongjoon-hyun commented May 26, 2019

Uh oh!

SparkQA commented May 26, 2019

Uh oh!

jzhuge commented May 26, 2019

Uh oh!

SparkQA commented May 27, 2019

Uh oh!

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 27, 2019

Uh oh!

Uh oh!

Uh oh!

rdblue commented May 27, 2019

Uh oh!

jzhuge commented May 28, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented May 28, 2019

Uh oh!

jzhuge commented May 28, 2019

Uh oh!

SparkQA commented May 28, 2019

Uh oh!

rdblueMay 28, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzhugeMay 29, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rdblueMay 29, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 29, 2019

Uh oh!

rdblue commented May 29, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented May 30, 2019

Uh oh!

jzhuge commented May 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jzhuge commented May 23, 2019•
edited
Loading

jzhuge commented May 28, 2019•
edited
Loading

rdblueMay 28, 2019•
edited
Loading

jzhugeMay 29, 2019•
edited
Loading

rdblue commented May 29, 2019•
edited
Loading