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.

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

[#5744]feat(catalog-model): Add a basic catalog-model framework in Gravitino #5757

Merged
merged 8 commits into from
Dec 9, 2024

Conversation

jerryshao
Copy link
Contributor

What changes were proposed in this pull request?

This PR adds the basic catalog-model framework in Gravitino. In the meantime, it also makes provider optional for built-in catalog.

Why are the changes needed?

Fix: #5744

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Tests will be added later on.

@jerryshao jerryshao requested review from yuqi1129 and mchades December 4, 2024 08:47
@jerryshao jerryshao self-assigned this Dec 4, 2024
mchades
mchades previously approved these changes Dec 5, 2024
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.

* catalog should be created. The short name:
*
* <p>1) should be the same as the {@link CatalogProvider} interface provided. 2) can be "null" if
* the created catalog is a built-in catalog, like model catalog.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm very puzzle about the concept of built-in catalog, can you explain it more clearly.

Copy link
Contributor

@yuqi1129 yuqi1129 Dec 5, 2024

Choose a reason for hiding this comment

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

And, so the provider of all built-in catalogs is null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me add more comment to explain the concept introduced here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the user level, the "provider" value can be null, user doesn't need to provide the "provider" for those catalogs.

From the Gravitino kernel, it will treat the catalog "type" name as the "provider" name, and use this name to load in the catalog.

@jerryshao jerryshao requested review from yuqi1129 and mchades December 9, 2024 02:03
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.

Overall LGTM.

@yuqi1129 yuqi1129 merged commit 02572aa into apache:main Dec 9, 2024
26 checks passed
Abyss-lord pushed a commit to Abyss-lord/gravitino that referenced this pull request Dec 13, 2024
… in Gravitino (apache#5757)

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

This PR adds the basic catalog-model framework in Gravitino. In the
meantime, it also makes `provider` optional for built-in catalog.

### Why are the changes needed?

Fix: apache#5744 

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

No.

### How was this patch tested?

Tests will be added later on.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Subtask] Add the basic module for model catalog
3 participants