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

[#3403] fix(hive-catalog): add hive catalog property list-all-tables #3703

Merged
merged 10 commits into from
Jun 5, 2024
Merged

[#3403] fix(hive-catalog): add hive catalog property list-all-tables #3703

merged 10 commits into from
Jun 5, 2024

Conversation

mygrsun
Copy link
Contributor

@mygrsun mygrsun commented May 31, 2024 •
edited
Loading

What changes were proposed in this pull request?

Add a Hive catalog property "list-all-tables". Using this property to control whether the Iceberg table is displayed in the Hive table list.

Why are the changes needed?

The bug is a schema has the Iceberg tables in the Hive catalog

Fix: #3403

Does this PR introduce any user-facing change?

N/A.

How was this patch tested?

1.create a hive catalog with "list-all-tables " property.
2.crate a database and a iceberg table in the catalog by hive beeline
3.check whether the table is displayed in the catalog .

@jerryshao
Copy link
Contributor

Can you please fix the style issue?

@FANNG1
Copy link
Contributor

FANNG1 commented Jun 1, 2024

It's recommended to add a test in CatalogHiveIT. you need to create an Iceberg catalog and creating Iceberg tables, you could refer CatalogIcebergBaseIT, but it depends on you since time is limited.

@FANNG1
Copy link
Contributor

FANNG1 commented Jun 1, 2024

You could use ./gradlew spotlessApply to fix the style to pass the CI

@FANNG1
Copy link
Contributor

FANNG1 commented Jun 1, 2024

could you fix the ci errror?

mchades
mchades reviewed Jun 2, 2024
View reviewed changes
...alog-hive/src/main/java/com/datastrato/gravitino/catalog/hive/HiveCatalogPropertiesMeta.java
@@ -36,6 +36,10 @@ public class HiveCatalogPropertiesMeta extends BaseCatalogPropertiesMetadata {

public static final String FETCH_TIMEOUT_SEC = "kerberos.keytab-fetch-timeout-sec";

public static final String LIST_ALL_TABLES = "list-all-tables";

public static final boolean DEFAULT_LIST_ALL_TABLES = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Due to potential performance issues that table filters may introduce when there are many tables, we should set the default value to true, and the default true also align with Hive client behavior.

Copy link
Contributor Author

@mygrsun mygrsun Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I took @FANNG1 advice ，set this to be false。so, @FANNG1 how do you think this advice?

Copy link
Contributor

Choose a reason for hiding this comment

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

talked with @mchades, we could remove the list-all-tables parameters and the default action is list only hive tables because we already need to get table meta data when listing table for now. If encountering performance issues we could add a parameter to control whether get all tables, but we could do track it in another issue.

@mchades
Copy link
Contributor

mchades commented Jun 5, 2024

It's recommended to add a test in CatalogHiveIT. you need to create an Iceberg catalog and creating Iceberg tables, you could refer CatalogIcebergBaseIT, but it depends on you since time is limited.

Overall LGTM, could you please resolve this comment?

@mygrsun
Copy link
Contributor Author

mygrsun commented Jun 5, 2024

It's recommended to add a test in CatalogHiveIT. you need to create an Iceberg catalog and creating Iceberg tables, you could refer CatalogIcebergBaseIT, but it depends on you since time is limited.

I don't have time to do this, I have tested this feature manualy. So ,can it be merged in advance？

@mygrsun
Copy link
Contributor Author

mygrsun commented Jun 5, 2024

It's recommended to add a test in CatalogHiveIT. you need to create an Iceberg catalog and creating Iceberg tables, you could refer CatalogIcebergBaseIT, but it depends on you since time is limited.

Overall LGTM, could you please resolve this comment?

thankx,

@mygrsun mygrsun closed this Jun 5, 2024
@mygrsun mygrsun reopened this Jun 5, 2024
@mchades
Copy link
Contributor

mchades commented Jun 5, 2024

It's recommended to add a test in CatalogHiveIT. you need to create an Iceberg catalog and creating Iceberg tables, you could refer CatalogIcebergBaseIT, but it depends on you since time is limited.

I don't have time to do this, I have tested this feature manualy. So ,can it be merged in advance？

If so, could you please open a new issue to track this? thanks!

@mygrsun mygrsun mentioned this pull request Jun 5, 2024
Closed
@mygrsun
Copy link
Contributor Author

mygrsun commented Jun 5, 2024

It's recommended to add a test in CatalogHiveIT. you need to create an Iceberg catalog and creating Iceberg tables, you could refer CatalogIcebergBaseIT, but it depends on you since time is limited.

I don't have time to do this, I have tested this feature manualy. So ,can it be merged in advance？

If so, could you please open a new issue to track this? thanks!

#3783
I add this one.

mchades
mchades approved these changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@mchades mchades left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contributions!

@FANNG1 do you want to take another look?

@FANNG1
Copy link
Contributor

FANNG1 commented Jun 5, 2024

LGTM

@yuqi1129 yuqi1129 merged commit 1952737 into apache:main Jun 5, 2024
69 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 5, 2024
@mygrsun @mygrsun2 @web-flow
[#3403] fix(hive-catalog): add hive catalog property list-all-tables (#…
5f8bdf6 
…3703)

<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
   Examples:
     - "[#123] feat(operator): support xxx"
     - "[#233] fix: check null before access result in xxx"
     - "[MINOR] refactor: fix typo in variable name"
     - "[MINOR] docs: fix typo in README"
     - "[#255] test: fix flaky test NameOfTheTest"
   Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. If the PR is unfinished, please mark this PR as draft.
-->

### What changes were proposed in this pull request?

Add a Hive catalog property "list-all-tables". Using this property to
control whether the Iceberg table is displayed in the Hive table list.

### Why are the changes needed?

The bug is a schema has the Iceberg tables in the Hive catalog

Fix: #3403

### Does this PR introduce _any_ user-facing change?

N/A.

### How was this patch tested?

1.create a hive catalog with "list-all-tables " property.
2.crate a database and a iceberg table  in the catalog by hive beeline
3.check whether the table is displayed in the catalog .

---------

Co-authored-by: ericqin <[email protected]>
diqiu50 pushed a commit to diqiu50/gravitino that referenced this pull request Jun 13, 2024
@mygrsun @mygrsun2 @diqiu50
[apache#3403] fix(hive-catalog): add hive catalog property list-all-t…
bf03743 
…ables (apache#3703)

<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
   Examples:
     - "[apache#123] feat(operator): support xxx"
     - "[apache#233] fix: check null before access result in xxx"
     - "[MINOR] refactor: fix typo in variable name"
     - "[MINOR] docs: fix typo in README"
     - "[apache#255] test: fix flaky test NameOfTheTest"
   Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. If the PR is unfinished, please mark this PR as draft.
-->

### What changes were proposed in this pull request?

Add a Hive catalog property "list-all-tables". Using this property to
control whether the Iceberg table is displayed in the Hive table list.

### Why are the changes needed?

The bug is a schema has the Iceberg tables in the Hive catalog

Fix: apache#3403

### Does this PR introduce _any_ user-facing change?

N/A.

### How was this patch tested?

1.create a hive catalog with "list-all-tables " property.
2.crate a database and a iceberg table  in the catalog by hive beeline
3.check whether the table is displayed in the catalog .

---------

Co-authored-by: ericqin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qqqttt123 qqqttt123 qqqttt123 left review comments

@yuqi1129 yuqi1129 yuqi1129 left review comments

@mchades mchades mchades approved these changes

@FANNG1 FANNG1 FANNG1 approved these changes

Assignees

@mygrsun mygrsun

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug report] hive catalog include iceberg table？
7 participants
@mygrsun @jerryshao @FANNG1 @mchades @yuqi1129 @qqqttt123 @mygrsun2