Skip to content

ARROW-4018: [C++] Fix RLE tests' failures on big-endian platforms#7144

Closed
kiszk wants to merge 2 commits intoapache:masterfrom
kiszk:ARROW-4018
Closed

ARROW-4018: [C++] Fix RLE tests' failures on big-endian platforms#7144
kiszk wants to merge 2 commits intoapache:masterfrom
kiszk:ARROW-4018

Conversation

@kiszk
Copy link
Member

@kiszk kiszk commented May 10, 2020

This PR adds big-endian support in RLE related classes. The data for RLE is stored using a little-endian format. The current code assumes a little-endian CPU in two places.

  1. RleDecoder::NextCounts (fix was proposed in ARROW-4018)
  2. packing and unpacking from https://github.com/lemire/FrameOfReference/blob/6ccaf9e97160f9a3b299e23a8ef739e711ef0c71/src/bpacking.cpp

After applying #7136 & this PR, the following failures in arrow-utility-test's failures will be fixed.

[  FAILED  ] Rle.SpecificSequences
[  FAILED  ] Rle.TestValues
[  FAILED  ] BitRle.Flush
[  FAILED  ] BitRle.Random
[  FAILED  ] BitRle.RepeatedPattern
[  FAILED  ] BitRle.Overflow
[  FAILED  ] RleDecoder.GetBatchSpaced

@github-actions
Copy link

@kiszk
Copy link
Member Author

kiszk commented May 11, 2020

This failure may not be related to this PR.

Copy link
Member

@pitrou pitrou 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. Just a question below.

@@ -31,6 +31,7 @@ namespace internal {

inline const uint32_t* unpack1_32(const uint32_t* in, uint32_t* out) {
uint32_t inl = util::SafeLoad(in);
inl = arrow::BitUtil::ToLittleEndian(inl);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be FromLittleEndian? (it will produce the same code, but it seems logically more accurate)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, you are right! I will update it soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@pitrou pitrou closed this in 25299e5 May 11, 2020
@pitrou
Copy link
Member

pitrou commented May 11, 2020

Thank you very much @kiszk !

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.

2 participants