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

ORC-1315:[C++] Fix byte to integer conversions fail on platforms with unsigned char type #1323

Merged
merged 2 commits into from Nov 25, 2022

Conversation

luffy-zh
Copy link
Contributor

@luffy-zh luffy-zh commented Nov 23, 2022

What changes were proposed in this pull request?

This patch adds static_cast for the char type cast in RLE/ColumnReader.

Why are the changes needed?

For the context of ORC-1315, this patch fixes signed char to unsigned char conversions fail where char is by default unsigned.

How was this patch tested?

No new tests were added.

@github-actions github-actions bot added the CPP label Nov 23, 2022
@dongjoon-hyun dongjoon-hyun changed the title ORC-1315:[C++] Fix byte to integer conversions fail on platforms with… ORC-1315:[C++] Fix byte to integer conversions fail on platforms with unsigned char type Nov 23, 2022
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Thank you for making a PR, @luffy-zh . BTW, according to the JIRA, do we need this at Apache ORC 1.8.1?

cc @wgtmac , @stiga-huang , @williamhyun .

@dongjoon-hyun dongjoon-hyun added this to the 1.8.1 milestone Nov 23, 2022
c++/src/ByteRLE.cc Show resolved Hide resolved
@wgtmac
Copy link
Member

wgtmac commented Nov 24, 2022

I believe we need to port to 1.8.1. @dongjoon-hyun

@dongjoon-hyun
Copy link
Member

Got it. Thanks, @wgtmac .

Copy link
Contributor

@coderex2522 coderex2522 left a comment

Choose a reason for hiding this comment

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

+1, LGTM.

c++/src/ColumnReader.cc Outdated Show resolved Hide resolved
@wgtmac wgtmac merged commit acb8d6c into apache:main Nov 25, 2022
@dongjoon-hyun
Copy link
Member

Hi, @wgtmac . Is there a reason why this is not backported to branch-1.8?

I believe we need to port to 1.8.1. @dongjoon-hyun

And, do we need 2 patches to be complete for ORC-1315?

$ git log --oneline main | grep ORC-1315
91ded769 ORC-1315: [C++] Fix test failure when unsigned char is in effect
acb8d6ca ORC-1315: [C++] Fix byte to integer conversion failure on platforms with unsigned char type

@dongjoon-hyun
Copy link
Member

Sorry, I was confused. I found your comment, #1318 (comment)

I can confirm that this issue only exists in the main branch, therefore we don't need backport the fix to branch-1.8.

I'll change the milestone of this PR.

@dongjoon-hyun dongjoon-hyun modified the milestones: 1.8.1, 1.9.0 Nov 29, 2022
cxzl25 pushed a commit to cxzl25/orc that referenced this pull request Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants