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

[Improvement] Unify the modification behavior of the comment field. #4968

Closed
mchades opened this issue Sep 20, 2024 · 9 comments · Fixed by #5121
Closed

[Improvement] Unify the modification behavior of the comment field. #4968

mchades opened this issue Sep 20, 2024 · 9 comments · Fixed by #5121
Assignees
@koonchen
Labels
0.7.0 Release v0.7.0 good first issue Good for newcomers improvement Improvements on everything

Comments

@mchades
Copy link
Contributor

mchades commented Sep 20, 2024

What would you like to be improved?

Currently, only the fileset supports the removeComment change; other entities like catalog and table only support updateComment changes. Additionally, newComment cannot be null or an empty string. We should unify this behavior.

How should we improve?

Unify the modification behavior of the comment field：

  • deprecated the removeComment change
  • the new comment of updateComment supports null and empty string
@mchades mchades added improvement Improvements on everything good first issue Good for newcomers labels Sep 20, 2024
@koonchen
Copy link
Contributor

@mchades i want to have a try.

@mchades
Copy link
Contributor Author

mchades commented Sep 23, 2024

@koonchen No problem! Please go ahead and feel free to ping me if you have any questions

@AbhiSharmaNIT
Copy link

Deprecate the removeComment change across all entities.
Reasoning: With the new behavior of updateComment (detailed below), removal of comments can be handled using this existing operation instead of a separate change type.

@AbhiSharmaNIT
Copy link

Allow null or empty string as valid values for newComment in updateComment.
If newComment is null or an empty string, it should be interpreted as removing the comment.

@mchades
Copy link
Contributor Author

mchades commented Oct 7, 2024

@AbhiSharmaNIT Yeah, you summed it up correctly!

I have not seen @koonchen submit the relevant PR so far, so I think if you are interested in this issue, you can also submit a PR.

@koonchen
Copy link
Contributor

koonchen commented Oct 8, 2024

@mchades this week is a national holiday, and you didn't assign me

@mchades
Copy link
Contributor Author

mchades commented Oct 8, 2024

@mchades this week is a national holiday, and you didn't assign me

assign is not a required step, you can start at any time.

@koonchen
Copy link
Contributor

koonchen commented Oct 8, 2024

@mchades I don't think so. You need to let others know the urgency and responsibility of the matter.

@mchades
Copy link
Contributor Author

mchades commented Oct 8, 2024

@mchades I don't think so. You need to let others know the urgency and responsibility of the matter.

I think the comments on the issue are sufficient. If you care about this, it has been assigned to you now, so please start as soon as possible.

Others can also submit at any time before relevant PRs are submitted.

koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 12, 2024
[apache#4968] Unify the modification behavior of the comment field (a…
990edd2 
…pache#4968)
@koonchen koonchen mentioned this issue Oct 12, 2024
Merged
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 12, 2024
[apache#4968] Unify the modification behavior of the comment field (a…
9c3c482 
…pache#4968)
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 12, 2024
[apache#4968] Unify the modification behavior of the comment field (a…
06681cd 
…pache#4968)
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 19, 2024
[apache#4968] improvement(api): Unify the modification behavior of th…
a664656 
…e comment field (apache#4968)
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 20, 2024
[apache#4968] improvement(api): Unify the modification behavior of th…
3fae0cc 
…e comment field (apache#4968)
This was referenced Oct 22, 2024
Closed
Closed
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 26, 2024
[apache#4968] improvement(api): Unify the modification behavior of th…
fa2fd28 
…e comment field (apache#4968)
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 26, 2024
[apache#4968] improvement(api): Unify the modification behavior of th…
c7446a1 
…e comment field (apache#4968)
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 26, 2024
[apache#4968] improvement(api): Unify the modification behavior of th…
9945d94 
…e comment field (apache#4968)
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 26, 2024
[apache#4968] improvement(api): Unify the modification behavior of th…
32662cf 
…e comment field (apache#4968)
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 27, 2024
[apache#4968] improvement(api): Unify the modification behavior of th…
53f45db 
…e comment field (apache#4968)
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 27, 2024
[apache#4968] improvement(api): Unify the modification behavior of th…
0983458 
…e comment field (apache#4968)
@koonchen koonchen mentioned this issue Oct 27, 2024
Open
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 27, 2024
[apache#4968] improvement(api): Unify the modification behavior of th…
930a4ad 
…e comment field (apache#4968)
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 27, 2024
[apache#4968] improvement(api): Unify the modification behavior of th…
4b029a3 
…e comment field (apache#4968)
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 27, 2024
[apache#4968] improvement(api): Unify the modification behavior of th…
9ed1ecd 
…e comment field (apache#4968)
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 27, 2024
[apache#4968] improvement(api): Unify the modification behavior of th…
b23de8f 
…e comment field (apache#4968)
koonchen pushed a commit to koonchen/gravitino that referenced this issue Oct 28, 2024
[apache#4968] improvement(api): Unify the modification behavior of th…
b1029ce 
…e comment field (apache#4968)
@mchades mchades added the 0.7.0 Release v0.7.0 label Oct 28, 2024
github-actions bot pushed a commit that referenced this issue Oct 28, 2024
@koonchen @web-flow
[#4968] improvement(api): Unify the modification behavior of the comm…
3ec69f9 
…ent field (#5121)

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

- deprecated the removeComment change
- the new comment of updateComment supports null and empty string

### Why are the changes needed?

Fix: #4968 

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

N/A

### How was this patch tested?

Pass existing tests

Co-authored-by: chenzeping.ricco <[email protected]>
@github-actions github-actions bot mentioned this issue Oct 28, 2024
Merged
jerryshao pushed a commit that referenced this issue Oct 28, 2024
@github-actions @koonchen
[#4968] improvement(api): Unify the modification behavior of the comm…
76dd6cc 
…ent field (#5288)

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

- deprecated the removeComment change
- the new comment of updateComment supports null and empty string

### Why are the changes needed?

Fix: #4968 

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

N/A

### How was this patch tested?

Pass existing tests

Co-authored-by: Ricco Chen <[email protected]>
Co-authored-by: chenzeping.ricco <[email protected]>
mplmoknijb pushed a commit to mplmoknijb/gravitino that referenced this issue Nov 6, 2024
@koonchen @mplmoknijb
[apache#4968] improvement(api): Unify the modification behavior of th…
cd5fbcc 
…e comment field (apache#5121)

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

- deprecated the removeComment change
- the new comment of updateComment supports null and empty string

### Why are the changes needed?

Fix: apache#4968 

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

N/A

### How was this patch tested?

Pass existing tests

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

@koonchen koonchen

Labels
0.7.0 Release v0.7.0 good first issue Good for newcomers improvement Improvements on everything
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[#4968] improvement(api): Unify the modification behavior of the comment field koonchen/gravitino
3 participants
@koonchen @mchades @AbhiSharmaNIT