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

[#1544] fix(hive): fix null comment in hive table property #1553

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

mchades
Copy link
Contributor

@mchades mchades commented Jan 17, 2024

What changes were proposed in this pull request?

set empty string as default value of comment when creating table

Why are the changes needed?

Fix: #1544

Does this PR introduce any user-facing change?

no

How was this patch tested?

IT added

@mchades mchades requested a review from FANNG1 January 17, 2024 07:09
@mchades mchades self-assigned this Jan 17, 2024
@FANNG1
Copy link
Contributor

FANNG1 commented Jan 17, 2024

How about set the default empty string to comment, then other catalogs doesn't need to handle null, and this also fixs #1546 , and more generally we'd better remove all null values when receiving DTO in Gravitino, @jerryshao , what do you think?

@jerryshao
Copy link
Contributor

jerryshao commented Jan 18, 2024

How about set the default empty string to comment, then other catalogs doesn't need to handle null, and this also fixs #1546 , and more generally we'd better remove all null values when receiving DTO in Gravitino, @jerryshao , what do you think?

Yeah, I think it is a more robust way other than a point fix, but still we have to handle empty string comment, right?

@mchades
Copy link
Contributor Author

mchades commented Jan 18, 2024

Giving an empty string as a default value of the comment field is a good way. Then we only need to deal with the null value in the comment assignment at first time.

@mchades mchades force-pushed the npe branch 3 times, most recently from 57e3e76 to bb8c920 Compare January 18, 2024 08:09
@mchades mchades changed the title [#1544] fix(hive): fix null comment in hive table property [#1544] improve(common): set empty string as default value of comment when creating table Jan 18, 2024
@mchades
Copy link
Contributor Author

mchades commented Jan 18, 2024

I have updated the PR, could you please review it again, thanks!
@FANNG1 @jerryshao

@mchades mchades requested a review from jerryshao January 18, 2024 08:20
FANNG1
FANNG1 previously approved these changes Jan 18, 2024
@FANNG1
Copy link
Contributor

FANNG1 commented Jan 18, 2024

LGTM

@@ -83,7 +83,7 @@ public TableCreateRequest(
@Nullable Partitioning[] partitioning) {
this.name = name;
this.columns = columns;
this.comment = comment;
this.comment = comment == null ? "" : comment;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we fix this in the server side?

@mchades mchades changed the title [#1544] improve(common): set empty string as default value of comment when creating table [#1544] fix(hive): fix null comment in hive table property Jan 18, 2024
@mchades mchades requested review from jerryshao and FANNG1 January 18, 2024 13:14
@jerryshao jerryshao merged commit 16e9f7c into apache:main Jan 18, 2024
7 checks passed
mchades added a commit to mchades/gravitino that referenced this pull request Jan 24, 2024
…che#1553)

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

set empty string as default value of comment when creating table

### Why are the changes needed?

Fix: apache#1544 

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

### How was this patch tested?

IT added
@mchades mchades deleted the npe branch November 22, 2024 03:38
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] createTable HiveTable failed if comment is null
3 participants