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

[hive] In hiveCatalog we do not need catalog.warehouse config #2408

Closed
wants to merge 1 commit into from
Closed

[hive] In hiveCatalog we do not need catalog.warehouse config #2408

wants to merge 1 commit into from

Conversation

monkeyboy123
Copy link
Contributor

Purpose

Linked issue: close #2407

In hiveCatalog we do not need catalog.warehouse config, it makes confused

Tests

API and Format

Documentation

@monkeyboy123
Copy link
Contributor Author

gentle ping @JingsongLi

JingsongLi
JingsongLi requested changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@JingsongLi JingsongLi left a comment

Choose a reason for hiding this comment

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

Can you modify CatalogFactory.createCatalog too? warehousePath and FileIO can be nullable.

And if the warehouse is null, we can get warehouse from HiveConf?

@JingsongLi
Copy link
Contributor

I think the correct way is hiveConf.get(HiveConf.ConfVars.METASTOREWAREHOUSE.varname).

@JingsongLi
Copy link
Contributor

And we need to enrich warehouse path to default scheme of conf if the path does not contain scheme.

@monkeyboy123
Copy link
Contributor Author

monkeyboy123 commented Nov 29, 2023 •
edited
Loading

I think the correct way is hiveConf.get(HiveConf.ConfVars.METASTOREWAREHOUSE.varname).

It is a good choice, but if we do it like this, we should let user set hive-conf-dir or set env HIVE_CONF_DIR or putting hive-site.xml into classapth.

@monkeyboy123
Copy link
Contributor Author

And we need to enrich warehouse path to default scheme of conf if the path does not contain scheme.

I will do it later.

@monkeyboy123 monkeyboy123 force-pushed the hivecatalog-warehouse-verify branch 3 times, most recently from a0cde26 to af17796 Compare November 29, 2023 05:30
@monkeyboy123 monkeyboy123 force-pushed the hivecatalog-warehouse-verify branch from af17796 to a41caa8 Compare November 29, 2023 06:17
@monkeyboy123
Copy link
Contributor Author

done @JingsongLi

@JingsongLi
Copy link
Contributor

Hi @monkeyboy123 , can you take a look to #2432 , is that satisfy your requirement?

@monkeyboy123
Copy link
Contributor Author

monkeyboy123 commented Dec 1, 2023 •
edited
Loading

Hi @monkeyboy123 , can you take a look to #2432 , is that satisfy your requirement?

I will take a look later.

@JingsongLi JingsongLi closed this Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JingsongLi JingsongLi JingsongLi requested changes

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

Successfully merging this pull request may close these issues.

[Feature] In hiveCatalog we do not need catalog.warehouse config
2 participants
@monkeyboy123 @JingsongLi