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

[#446] feat(iceberg): Support detailed iceberg catalog property metadata #486

Merged
merged 3 commits into from
Oct 16, 2023
Merged

[#446] feat(iceberg): Support detailed iceberg catalog property metadata #486

merged 3 commits into from
Oct 16, 2023

Conversation

Clearvive
Copy link
Contributor

@Clearvive Clearvive commented Oct 10, 2023 •
edited by yuqi1129
Loading

What changes were proposed in this pull request?

Implement catalogPropertiesMetadata、tablePropertiesMetadata for IcebergCatalogOperations

Why are the changes needed?

Currently catalogPropertiesMetadatatablePropertiesMetadata will return an empty map, and we need to implement it to return a real value for the iceberg property metadata.

Fix: #446

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Add test testCatalogProperty in TestIcebergCatalog
Add test testTableProperty in TestIcebergTable

@github-actions
Copy link

github-actions bot commented Oct 10, 2023 •
edited
Loading

Code Coverage Report

Overall Project 62.14% -0.03% 🟢
Files changed 94.44% 🟢

Module Coverage
catalog-lakehouse-iceberg 81.02% -0.34% 🟢
Files
Module File Coverage
catalog-lakehouse-iceberg IcebergCatalogBackend.java 100% 🟢
IcebergConfig.java 100% 🟢
IcebergTablePropertiesMetadata.java 100% 🟢
IcebergTableOps.java 90.64% 🟢
IcebergCatalogUtil.java 85.95% 🟢
IcebergCatalogPropertiesMetadata.java 82.09% -17.91% 🟢
IcebergCatalogOperations.java 77.96% -0.24% 🟢

@Clearvive Clearvive requested a review from FANNG1 October 10, 2023 07:29
@Clearvive Clearvive self-assigned this Oct 10, 2023
@Clearvive Clearvive added this to the Graviton v0.2.0 milestone Oct 10, 2023
@Clearvive Clearvive force-pushed the 446/iceberg/prop-v1 branch 2 times, most recently from 120cf90 to 3771e55 Compare October 10, 2023 07:40
@jerryshao jerryshao removed this from the Graviton v0.2.0 milestone Oct 10, 2023
@Clearvive Clearvive force-pushed the 446/iceberg/prop-v1 branch 3 times, most recently from 96e097a to ebb3e62 Compare October 10, 2023 10:04
@yuqi1129
Copy link
Contributor

  1. Please modify your PR description to match the specifications.
  2. Is it correct that you haven't added UT for the Iceberg table property?

@Clearvive
Copy link
Contributor Author

  1. Please modify your PR description to match the specifications.
  2. Is it correct that you haven't added UT for the Iceberg table property?

Yes, we are currently in the phase of discussing which properties need to be added.

@Clearvive Clearvive requested a review from FANNG1 October 12, 2023 02:45
@Clearvive Clearvive marked this pull request as ready for review October 12, 2023 09:37
@FANNG1
Copy link
Contributor

FANNG1 commented Oct 12, 2023

  1. suggest using catalog-type, uri, warehouse, jdbc-user, jdbc-password as graviton keyword to let user use it simple.
  2. for bypass properties, using graviton.bypass.iceberg. to passby to iceberg properties, graviton.bypass.hadoop to hadoop configurations.

jerryshao
jerryshao reviewed Oct 13, 2023
View reviewed changes
Copy link
Contributor

@jerryshao jerryshao left a comment

Choose a reason for hiding this comment

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

@sandflee would you please take a review.

...java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogPropertiesMetadata.java Outdated Show resolved Hide resolved
...e-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergConfig.java Outdated Show resolved Hide resolved
...rc/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java Show resolved Hide resolved
...berg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogType.java Outdated Show resolved Hide resolved
...java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogPropertiesMetadata.java Show resolved Hide resolved
...e-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergConfig.java Outdated Show resolved Hide resolved
@jerryshao
Copy link
Contributor

Another thing is that: for lakehouse-iceberg, we have two entry points (Iceberg rest service and Graviton catalog system) and two config systems, we should carefully think of how to unify this two config mechanism.

@Clearvive Clearvive requested a review from FANNG1 October 13, 2023 08:36
@Clearvive Clearvive requested a review from FANNG1 October 16, 2023 06:31
@FANNG1
Copy link
Contributor

FANNG1 commented Oct 16, 2023

Generally LGTM, I suggest adding properties test in integrate test later, for the properties logic lay across serval modules.

FANNG1
FANNG1 previously approved these changes Oct 16, 2023
View reviewed changes
@jerryshao
Copy link
Contributor

@yuqi1129 Can you please help to review?

@yuqi1129
Copy link
Contributor

The rest are LGTM.

@jerryshao jerryshao merged commit 3ce6cf4 into apache:main Oct 16, 2023
2 checks passed
Clearvive added a commit that referenced this pull request Oct 17, 2023
@Clearvive
[#446] feat(iceberg): Support detailed iceberg catalog property metad…
805be9d 
…ata (#486)

### What changes were proposed in this pull request?
Implement catalogPropertiesMetadata、tablePropertiesMetadata for
IcebergCatalogOperations

### Why are the changes needed?
Currently catalogPropertiesMetadatatablePropertiesMetadata will return
an empty map, and we need to implement it to return a real value for the
iceberg property metadata.

Fix: #446 

### Does this PR introduce any user-facing change?
N/A

### How was this patch tested?
Add test testCatalogProperty in TestIcebergCatalog
Add test testTableProperty in TestIcebergTable
jerryshao pushed a commit that referenced this pull request Oct 26, 2023
@Clearvive
[#446] feat(iceberg): Support detailed iceberg catalog property metad…
16d0075 
…ata (#486)

### What changes were proposed in this pull request?
Implement catalogPropertiesMetadata、tablePropertiesMetadata for
IcebergCatalogOperations

### Why are the changes needed?
Currently catalogPropertiesMetadatatablePropertiesMetadata will return
an empty map, and we need to implement it to return a real value for the
iceberg property metadata.

Fix: #446 

### Does this PR introduce any user-facing change?
N/A

### How was this patch tested?
Add test testCatalogProperty in TestIcebergCatalog
Add test testTableProperty in TestIcebergTable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuqi1129 yuqi1129 yuqi1129 left review comments

@jerryshao jerryshao jerryshao approved these changes

@FANNG1 FANNG1 FANNG1 left review comments

Assignees

@Clearvive Clearvive

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Subtask] Add Iceberg graviton properties support
4 participants
@Clearvive @yuqi1129 @FANNG1 @jerryshao