Fix assertion failure in CRoaring on self-merge of NumericIndexedVector#99976
Fix assertion failure in CRoaring on self-merge of NumericIndexedVector#99976Avogar merged 3 commits intoClickHouse:masterfrom
Conversation
…f-merge of NumericIndexedVector The `executeAggregateMultiply` function uses exponentiation by squaring, which calls `function->merge(vec_from[i], vec_from[i], arena)` (self-merge) on even iterations. For `BSINumericIndexedVector`, this calls `pointwiseAddInplace` where `this == &rhs`. Since `shallowCopyFrom` shares Roaring bitmap pointers, the full adder loop performs `sum->rb_xor(*addend)` on aliased bitmaps, triggering `assert(x1 != x2)` in debug builds and producing incorrect results (A XOR A = 0) in release builds. Add a self-merge guard to both `pointwiseAddInplace` and `pointwiseSubtractInplace` that deep-copies the argument when `this == &rhs`. ClickHouse#99704 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
f77a55f to
4fd576d
Compare
|
Hi @PedroTadim @alexey-milovidov @Algunenano |
| -- https://github.com/ClickHouse/ClickHouse/issues/99704 | ||
|
|
||
| -- `multiply` triggers self-merge via exponentiation by squaring (even branch). | ||
| SELECT arrayJoin([numericIndexedVectorToMap( |
There was a problem hiding this comment.
Yes, the test reproduces the issue.
|
Workflow [PR], commit [bf80514] Summary: ✅ AI ReviewSummaryThis PR fixes a real correctness bug in Missing context
ClickHouse Rules
Final Verdict
|
|
@Desel72, the test has failed. |
…ix/issue-99704
…pected output
The function signature is (index, value), so (100, 1) produces {100:2}
after multiply(2, ...), matching the reference file.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@alexey-milovidov I've solved this. |
I just don't see what are we missing here. |
@antaljanosbenjamin I made a mistake. Sorry for that. I mean you can consider this #99977 (comment)? |
|
Okay, let me try to address your comment.
I feel sorry about this, but there is nothing wrong about it. You have two PRs with the same changes. One of them has to be closed, it doesn't make sense to merge both of them, because they address the same issue as far as I can see.
Why would be a draft PR better than a closed one? It is better to close one of them and do not let it wonder around for the future.
You don't need to worry. You are very welcome to contribute to ClickHouse as anybody else. A bunch of people opened duplicate PRs, it is not a big deal. |
@antaljanosbenjamin Can I contact with you on telegram? |
LLVM Coverage Report
PR changed lines: PR changed-lines coverage: 71.43% (15/21, 0 noise lines excluded) |
|
Sorry, but I don't understand what is the problem, therefore I don't see any reason to contact me in private. |
Sorry for that. That is because of my personal problem.. |
…rge of NumericIndexedVector
…erge of NumericIndexedVector
…rge of NumericIndexedVector
…rge of NumericIndexedVector
…rge of NumericIndexedVector
Backport #99976 to 26.2: Fix assertion failure in CRoaring on self-merge of NumericIndexedVector
Backport #99976 to 26.1: Fix assertion failure in CRoaring on self-merge of NumericIndexedVector
Backport #99976 to 26.3: Fix assertion failure in CRoaring on self-merge of NumericIndexedVector
Backport #99976 to 25.12: Fix assertion failure in CRoaring on self-merge of NumericIndexedVector
Backport #99976 to 25.8: Fix assertion failure in CRoaring on self-merge of NumericIndexedVector
Fix assertion failure in CRoaring
roaring_bitmap_xor_inplaceon self-merge of NumericIndexedVectorCloses #99704
The
executeAggregateMultiplyuses exponentiation by squaring, which callsfunction->merge(vec_from[i], vec_from[i], arena)(self-merge) on even iterations. ForBSINumericIndexedVector, this callspointwiseAddInplacewherethis == &rhs. SinceshallowCopyFromshares Roaring bitmap pointers, the full adder loop performssum->rb_xor(*addend)on aliased bitmaps, triggeringassert(x1 != x2)in debug builds and producing incorrect results (A XOR A = 0) in release builds.Added a self-merge guard to both
pointwiseAddInplaceandpointwiseSubtractInplacethat deep-copies the argument whenthis == &rhs.Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix assertion failure (exception in debug builds, incorrect results in release builds) when multiplying
NumericIndexedVectoraggregate states by an even integer constant, caused by self-XOR on aliased Roaring bitmaps inpointwiseAddInplace.Documentation entry for user-facing changes