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

[#1759] feat(TableChange) : TableChange support index. #2359

Merged
merged 3 commits into from
Feb 28, 2024

Conversation

Clearvive
Copy link
Contributor

@Clearvive Clearvive commented Feb 27, 2024

What changes were proposed in this pull request?

TableChange support index.

The current PR does not support the extension of AlterIndex.
Currently, only AddIndex and DeleteIndex are supported.

Why are the changes needed?

Fix: #1759

Does this PR introduce any user-facing change?

Add Index TableChange

How was this patch tested?

IT,UT

@Clearvive Clearvive added this to the Gravitino 0.5.0 milestone Feb 27, 2024
@Clearvive Clearvive requested a review from yuqi1129 February 27, 2024 06:21
@Clearvive Clearvive self-assigned this Feb 27, 2024
@Clearvive Clearvive requested a review from FANNG1 February 27, 2024 06:22
@Clearvive Clearvive force-pushed the 1759/add-index-change branch from 08b2394 to b2592af Compare February 27, 2024 06:49
@Clearvive Clearvive requested a review from mchades February 27, 2024 06:49
@Clearvive Clearvive force-pushed the 1759/add-index-change branch from 023b8fd to b6d4049 Compare February 27, 2024 07:39
@Clearvive Clearvive requested a review from yuqi1129 February 27, 2024 09:49
@Clearvive Clearvive requested a review from FANNG1 February 27, 2024 10:12
@SteNicholas
Copy link
Member

SteNicholas commented Feb 27, 2024

@Clearvive, does this pull request support AlterIndex? If AlterIndex does not support, please add some detail of index support in descripition of this pull request.
BTW, does this pull request fix #1759 instead of #1758?

@Clearvive
Copy link
Contributor Author

@Clearvive, does this pull request support AlterIndex? If AlterIndex does not support, please add some detail of index support in descripition of this pull request. BTW, does this pull request fix #1759 instead of #1758?

This PR does not support AlterIndex. Currently, not all metadata supports modifying index information. Users can perform operations by first deleting and then creating anew. I will now modify the description of this issue accordingly.

@Clearvive Clearvive force-pushed the 1759/add-index-change branch from 416de74 to 4094332 Compare February 28, 2024 01:55
@Clearvive Clearvive force-pushed the 1759/add-index-change branch from b59eda9 to c38bca0 Compare February 28, 2024 06:20
yuqi1129
yuqi1129 previously approved these changes Feb 28, 2024
Copy link
Contributor

@yuqi1129 yuqi1129 left a comment

Choose a reason for hiding this comment

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

I'm okay with the current changes.

@FANNG1
Copy link
Contributor

FANNG1 commented Feb 28, 2024

LGTM except the minor comments

@Clearvive Clearvive merged commit 52ec733 into apache:main Feb 28, 2024
12 checks passed
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.

[FEATURE] TableChange needs to support index.
5 participants