fix(types): make RowKind short string parsing case-insensitive by slfan1989 · Pull Request #15 · apache/paimon-cpp · GitHub
Skip to content

fix(types): make RowKind short string parsing case-insensitive - #15

Merged
leaves12138 merged 2 commits into
apache:mainfrom
slfan1989:paimon-cpp-14
Jun 1, 2026
Merged

fix(types): make RowKind short string parsing case-insensitive#15
leaves12138 merged 2 commits into
apache:mainfrom
slfan1989:paimon-cpp-14

Conversation

@slfan1989

@slfan1989 slfan1989 commented May 26, 2026

Copy link
Copy Markdown
Contributor

Purpose

Linked issue: close #14

Make RowKind::FromShortString consistent with Java Paimon's RowKind.fromShortString behavior.

Java Paimon normalizes the input to uppercase before matching, so lowercase short strings such as +i, -u, +u, and -d are accepted. This PR updates paimon-cpp to accept the same lowercase variants.

Tests

Added unit test coverage for lowercase RowKind short strings:

  • +i maps to INSERT
  • -u maps to UPDATE_BEFORE
  • +u maps to UPDATE_AFTER
  • -d maps to DELETE

API and Format

No.
This change does not affect public API headers under include/, storage format, or protocol.

Documentation

No.
This is a compatibility bug fix and does not introduce a new feature.

Generative AI tooling

@slfan1989

slfan1989 commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

@lxy-9602

lxy-9602 commented May 26, 2026

Copy link
Copy Markdown
Member

Thank you for the contribution. This fix looks correct and addresses the case-sensitivity issue.

We have a utility function, StringUtils::ToUpperCase, that has not been merged yet. Would it be possible to wait until that utility is available? I think it could make the code a bit cleaner. We expect to merge it soon, and then sync it in this PR if that works for you. Thanks!

@slfan1989

slfan1989 commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for the contribution. This fix looks correct and addresses the case-sensitivity issue.

We have a utility function, StringUtils::ToUpperCase, that has not been merged yet. Would it be possible to wait until that utility is available? I think it could make the code a bit cleaner. We expect to merge it soon, and then sync it in this PR if that works for you. Thanks!

@lxy-9602 Thank you for the feedback. That sounds good to me.

I’m happy to wait until StringUtils::ToUpperCase is merged, and then I can update this PR to use the utility function to make the code cleaner.

Thanks again!

@lxy-9602

lxy-9602 commented May 29, 2026

Copy link
Copy Markdown
Member

@slfan1989 Hello, StringUtils has been merged in #23 . When you have time, could you please rebase your branch and update this PR accordingly? Thanks again for the fix!

@slfan1989

slfan1989 commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

@slfan1989 Hello, StringUtils has been merged in #23 . When you have time, could you please rebase your branch and update this PR accordingly? Thanks again for the fix!

@lxy-9602 Thanks for the reminder and for reviewing the fix!

I have rebased the branch onto the latest main and updated the PR to use StringUtils::ToUpperCase from #23. I also moved FromShortString into row_kind.cpp to avoid adding the StringUtils dependency to the header.

Verified the RowKind parsing behavior locally.

leaves12138
leaves12138 approved these changes Jun 1, 2026
View reviewed changes

@leaves12138 leaves12138 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed the latest RowKind change. Moving FromShortString to the .cpp and normalizing via StringUtils::ToUpperCase looks good, and the added lowercase short-string test covers the intended behavior. I do not see blockers.

@leaves12138
leaves12138 merged commit e595f72 into apache:main Jun 1, 2026
@slfan1989

slfan1989 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leaves12138 leaves12138 leaves12138 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] RowKind::FromShortString should be case-insensitive

3 participants

@slfan1989 @lxy-9602 @leaves12138