fix(core): fix data corruption in DECIMAL128 and DECIMAL256 columns#6873
fix(core): fix data corruption in DECIMAL128 and DECIMAL256 columns#6873bluestreak01 merged 10 commits intomasterfrom
Conversation
set_memory_vanilla_vec() delegates to run_vec_bulk, which uses TVec::size() as the loop increment. For Vec8uq that returns 8 (number of uint64_t lanes), but one 512-bit store only covers 4 long_128bit elements or 2 long_256bit elements. The mismatch causes the bulk path to advance past unwritten elements, leaving holes filled with stale data. Replace the broken run_vec_bulk call in set_memory_vanilla_vec with a dedicated loop that computes the correct per-store element count from sizeof(TVec) / sizeof(T), guarded by a static_assert. Add VectFuzzTest cases for setMemoryLong128 and setMemoryLong256 with poison-pattern pre-fill and multiple alignment offsets to exercise head, bulk, and tail paths.
jerrinot
added
Bug
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes.Add |
|
expecting tests in this run to fail. then I'll rebuild the native libs and the tests should pass |
There was a problem hiding this comment.
Pull request overview
This PR fixes SIMD-accelerated bulk fill for 128-bit and 256-bit “long_*bit” elements by correcting the vector-loop stride, and adds targeted regression tests to detect previously skipped/“hole” elements and sentinel corruption.
Changes:
- Fix
set_memory_vanilla_*vector bulk-fill logic forlong_128bitandlong_256bitby computing elements-per-store from vector byte width rather thanTVec::size(). - Add regression tests for
Vect.setMemoryLong128andVect.setMemoryLong256that validate all elements are written and no overflow occurs.
Reviewed changes
Copilot reviewed 2 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/src/main/c/share/ooo_dispatch.cpp | Reworks set_memory_vanilla_vec to use correct SIMD store stride for 128/256-bit element sizes. |
| core/src/test/java/io/questdb/test/std/VectFuzzTest.java | Adds regression tests for setMemoryLong128/256 to catch skipped elements and overflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
bluestreak01
changed the title
Add non-element-aligned offsets to the DECIMAL128/DECIMAL256 bulk-fill tests (offset 8 for 128-bit, offset 16 for 256-bit) so the pure-scalar fallback path in set_memory_vanilla_vec is exercised when unaligned % sizeof(T) != 0. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
set_memory_vanilla_vec() delegates to run_vec_bulk, which uses TVec::size() as the loop increment.
For Vec8uq that returns 8 (number of uint64_t lanes), but one 512-bit store only covers 4 long_128bit elements or 2 long_256bit elements. The mismatch causes the bulk path to advance past unwritten elements, leaving holes filled with stale data.
Replace the broken run_vec_bulk call in set_memory_vanilla_vec with a dedicated loop that computes the correct per-store element count from sizeof(TVec) / sizeof(T), guarded by a static_assert.
Add VectFuzzTest cases for setMemoryLong128 and setMemoryLong256 with poison-pattern pre-fill and multiple alignment offsets to exercise head, bulk, and tail paths.