Skip to content

ARROW-8974: [C++] Simplify TransferBitmap - #7373

Closed
cyb70289 wants to merge 3 commits into
apache:masterfrom
cyb70289:bitmap-transfer
Closed

ARROW-8974: [C++] Simplify TransferBitmap#7373
cyb70289 wants to merge 3 commits into
apache:masterfrom
cyb70289:bitmap-transfer

Conversation

@cyb70289

Copy link
Copy Markdown
Contributor

This patch removes "restore_trailing_bits" template parameter of
TransferBitmap class. Trailing bits are now always not clobbered,
which is no harm. It also refines trailing bits processing to keep
the performance influence trivial. Besides, this patch replaces
"invert_bits" boolean parameter with enum to allow explicit naming.

@github-actions

Copy link
Copy Markdown

@pitroupitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for the simplification. Just one suggestion.

Comment threadcpp/src/arrow/util/bitmap_ops.cc Outdated
@cyb70289

Copy link
Copy Markdown
ContributorAuthor

RTools CI failure looks not related
https://github.com/apache/arrow/pull/7373/checks?check_run_id=752286352#step:7:1077
error: failed retrieving file 'mingw-w64-i686-thrift-0.13.0-8000-any.pkg.tar.xz' from dl.bintray.com : Operation too slow. Less than 1 bytes/sec transferred the last 10 seconds

Comment threadcpp/src/arrow/util/bitmap_ops.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I meant use the kTrailingBitmask constant array that's exposed in bit_util.h :-)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ah, it's a lookup table, cool. Will change.

@cyb70289cyb70289Jun 9, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We need a table {255, 127, 63, 31, 15, 7, 3, 1}, looks there's no such lookup table available.
Shall I add a new table or keep the code as is (revert the renaming)?
https://github.com/apache/arrow/blob/master/cpp/src/arrow/util/bit_util.h#L420

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, let's simply revert the renaming then :-)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

@cyb70289

Copy link
Copy Markdown
ContributorAuthor

s390 ci failure is not related, it says "no space left on device".
appveyor ci failed with many zlib and lz4 runtime errors.

@wesm

wesm commented Jun 10, 2020

Copy link
Copy Markdown
Member

the Appveyor failures are ARROW-9085

This patch removes "restore_trailing_bits" template parameter of
TransferBitmap class. Trailing bits are now always not clobbered,
which is no harm. It also refines trailing bits processing to keep
the performance influence trivial. Besides, this patch replaces
"invert_bits" boolean parameter with enum to allow explicit naming.
This reverts commit 3a385542bea5954f2443f79d34a6a83c85e45f05.
@pitrou

Copy link
Copy Markdown
Member

Rebased to (hopefully) fix AppVeyor.

@pitrou

Copy link
Copy Markdown
Member

Thank you @cyb70289 !

sgnkc pushed a commit to sgnkc/arrow that referenced this pull request Jun 11, 2020
This patch removes "restore_trailing_bits" template parameter of
TransferBitmap class. Trailing bits are now always not clobbered,
which is no harm. It also refines trailing bits processing to keep
the performance influence trivial. Besides, this patch replaces
"invert_bits" boolean parameter with enum to allow explicit naming.
Closesapache#7373 from cyb70289/bitmap-transfer
Authored-by: Yibo Cai <yibo.cai@arm.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@cyb70289
cyb70289 deleted the bitmap-transfer branch June 14, 2020 06:47
Sign up for freeto 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.

3 participants

@cyb70289@wesm@pitrou