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

[#4540] fix(core): Support alter tag with null comment #4541

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

jerryshao
Copy link
Contributor

What changes were proposed in this pull request?

Add NULL check for tag comment

Why are the changes needed?

Fix: #4540

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add IT.

@jerryshao jerryshao self-assigned this Aug 15, 2024
@jerryshao jerryshao requested a review from yuqi1129 August 15, 2024 11:41
@yuqi1129
Copy link
Contributor

Could you also check if this problem exists in other entities, I noticed that there is a similar operation in metalake
https://github.com/apache/gravitino/blob/6acf9c92924a20924f8778e98ca3c3adea446fe6/core/src/main/java/org/apache/gravitino/storage/relational/mapper/MetalakeMetaMapper.java#L137C1-L138C1

@jerryshao
Copy link
Contributor Author

jerryshao commented Aug 15, 2024

Could you also check if this problem exists in other entities, I noticed that there is a similar operation in metalake https://github.com/apache/gravitino/blob/6acf9c92924a20924f8778e98ca3c3adea446fe6/core/src/main/java/org/apache/gravitino/storage/relational/mapper/MetalakeMetaMapper.java#L137C1-L138C1

I guess so, but I don't have time to test all of them. I can check it in a separate PR.

@yuqi1129
Copy link
Contributor

Could you also check if this problem exists in other entities, I noticed that there is a similar operation in metalake 6acf9c9/core/src/main/java/org/apache/gravitino/storage/relational/mapper/MetalakeMetaMapper.java#L137C1-L138C1

I guess so, but I don't have time to test all of them. I can check it in a separate PR.

OK

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.

@jerryshao jerryshao merged commit 62fbc73 into apache:main Aug 16, 2024
34 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 16, 2024
### What changes were proposed in this pull request?

Add NULL check for tag comment 

### Why are the changes needed?

Fix: #4540 

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

No.

### How was this patch tested?

Add IT.
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.

[Bug report] update tag failed
2 participants