GH-47825: [C++] Fix the issue that bitmap ops overriding partial leading byte#47912
GH-47825: [C++] Fix the issue that bitmap ops overriding partial leading byte#47912zanmato1984 merged 2 commits intoapache:mainfrom
Conversation
zanmato1984
added
the
Critical Fix
cpp/src/arrow/util/bitmap_test.cc
Outdated
|
|
||
| namespace arrow::internal { | ||
|
|
||
| TEST(BitmapOpsTest, PartialLeadingByteSafety) { |
There was a problem hiding this comment.
Since CopyBitmap and others are tested in bit_util_test.cc, should we move those tests here?
There was a problem hiding this comment.
Yeah, saw the bunch of cases in bit_util_test.cc are actually about bitmap_ops.cc. Plus the bit_util_test.cc are already over 2500 LOC. Will do.
There was a problem hiding this comment.
Moved a bunch of existing "bitmap" related tests (1000+ lines) from bit_util_test.cc to the new bitmap_test.cc. Now the PR is big enough to review :)
cpp/src/arrow/util/CMakeLists.txt
Outdated
| SOURCES | ||
| align_util_test.cc | ||
| atfork_test.cc | ||
| bitmap_test.cc |
There was a problem hiding this comment.
Would rather add this to bit-utility-test than to the already overcrowded utility-test.
There was a problem hiding this comment.
Yes, sounds right. Didn't see bit-utility-test is actually a collection of several test files. Will do.
|
@github-actions crossbow submit -g cpp |
github-actions
bot
added
awaiting committer review
|
Revision: 106c633 Submitted crossbow builds: ursacomputing/crossbow @ actions-4b57902598
|
|
@zanmato1984 In addition to the comments above, can you also rebase from git main? |
…test, and move bitmap tests from bit_util_test.cc into the new file
zanmato1984
force-pushed
the
fix/gh-47825
branch
from
1b2efc8 to
37a41be
Compare
Thanks for looking. Rebased. |
|
@github-actions crossbow submit -g cpp |
|
Revision: 37a41be Submitted crossbow builds: ursacomputing/crossbow @ actions-cf05f6aa35
|
|
Would you take another look? @pitrou thanks. |
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 2119a27. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…l leading byte (apache#47912) ### Rationale for this change `BitmapOp` overrides partial leading byte for unaligned offset. ### What changes are included in this PR? Fix by properly handle the leading byte in the fast path of `BitmapOp`. ### Are these changes tested? Bitmap op UT included. E2E test for `if_else` function execution incluced. ### Are there any user-facing changes? None. **This PR contains a "Critical Fix".** (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.) * GitHub Issue: apache#47825 Authored-by: Rossi Sun <zanmato1984@gmail.com> Signed-off-by: Rossi Sun <zanmato1984@gmail.com>
3 participants
Rationale for this change
BitmapOpoverrides partial leading byte for unaligned offset.What changes are included in this PR?
Fix by properly handle the leading byte in the fast path of
BitmapOp.Are these changes tested?
Bitmap op UT included.
E2E test for
if_elsefunction execution incluced.Are there any user-facing changes?
None.
This PR contains a "Critical Fix". (If the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld), please provide explanation. If not, you can remove this.)
if_elseemits wrong results for chunked inputs #47825