[SPARK-27693][SQL] Add default catalog property #24594
Conversation
|
@jzhuge, @mccheah, @cloud-fan, here is a PR that adds a config property to set the default v2 catalog. |
|
Test build #105366 has finished for PR 24594 at commit
|
|
@cloud-fan, can you take a look at this? Future PRs depend on the default catalog to resolve v2 tables. |
|
Test build #105386 has finished for PR 24594 at commit
|
rdblue
force-pushed
the
SPARK-27693-add-default-catalog-config
branch
from
d8e2849 to
03cf48b
Compare
|
I've rebased and updated this to enable the tests that were ignored in #24570. |
|
Test build #105421 has finished for PR 24594 at commit
|
|
@cloud-fan, any more comments? It would be great to get this in so that we can start using it. |
| @@ -1767,6 +1767,11 @@ object SQLConf { | |||
| "with String") | |||
| .booleanConf | |||
| .createWithDefault(false) | |||
|
|
|||
| val DEFAULT_V2_CATALOG = buildConf("spark.sql.default.catalog") | |||
| .doc("Name of the default v2 catalog, used when an catalog is not identified in queries") | |||
There was a problem hiding this comment.
nit. an catalog -> a catalog.
There was a problem hiding this comment.
as we discussed in the DS v2 meeting, we should clearly point out which places this default catalog is used. View/Function resolution definitely doesn't use this default catalog for now.
There was a problem hiding this comment.
I agree, we need to make clear choices and document where this is used.
|
Retest this please. |
There was a problem hiding this comment.
+1, LGTM. Thank you, @rdblue and @jzhuge .
I'll merge this to the master branch after fixing the typo.
cc @cloud-fan and @gatorsmile .
Add a SQL config property for the default v2 catalog. Existing tests for regressions. Closes #24594 from rdblue/SPARK-27693-add-default-catalog-config. Authored-by: Ryan Blue <blue@apache.org> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
|
Thanks @dongjoon-hyun ! |
| @@ -1767,6 +1767,11 @@ object SQLConf { | |||
| "with String") | |||
| .booleanConf | |||
| .createWithDefault(false) | |||
|
|
|||
| val DEFAULT_V2_CATALOG = buildConf("spark.sql.default.catalog") | |||
There was a problem hiding this comment.
nit: spark.sql.catalog.default is more consistent with other SQL config names: spark.sql.componentName.featureName.
There was a problem hiding this comment.
The reason for this order is that spark.sql.catalog.(name) properties are used to register catalogs. So the property name you suggest creates a catalog named "default" using the existing convention. We could change the properties to use catalogs instead if you'd prefer. Then the catalog.default name would not conflict.
There was a problem hiding this comment.
As @rdblue mentioned, spark.sql.catalog.default has meaning. Also, the spark.sql.default.catalog was the name already used in our code base before this PR.
There was a problem hiding this comment.
Ah I see the problem now.
Since we already have spark.sql.defaultSizeInBytes, shall we name it spark.sql.defaultCatalog?
There was a problem hiding this comment.
That works for me. I'll open a PR to rename.
|
Test build #105546 has finished for PR 24594 at commit
|
Add a SQL config property for the default v2 catalog. Existing tests for regressions. Closes apache#24594 from rdblue/SPARK-27693-add-default-catalog-config. Authored-by: Ryan Blue <blue@apache.org> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
### What changes were proposed in this pull request? rename the config to address the comment: #24594 (comment) improve the config description, provide a default value to simplify the code. ### Why are the changes needed? make the config more understandable. ### Does this PR introduce any user-facing change? no ### How was this patch tested? existing tests Closes #26395 from cloud-fan/config. Authored-by: Wenchen Fan <wenchen@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
Add a SQL config property for the default v2 catalog.
How was this patch tested?
Existing tests for regressions.