u70b3 commented on PR #66857: URL: https://github.com/apache/doris/pull/66857#issuecomment-5324539736
Thanks for the thorough review. Every finding was re-verified against the code and all of them held up. Everything is addressed in `4405d55a` (pushed to this branch). Details below. ## Blocking issues — fixed **1. VersionWithTime version/timestamp is not a consistent snapshot** Fixed by swapping the write order: `update_ts` is now stored (relaxed) *before* the release CAS that publishes the new version. The release/acquire chain on `version` then guarantees that any reader observing the new version subsequently loads a timestamp at least as new as the one stored for that version's publication — "new version + stale timestamp" is impossible, on x86 as well. The residual "old version + newer timestamp" combination only biases compaction toward the conservative max retention count. The struct comment now states this exact invariant instead of the previous overclaim. Added a deterministic concurrency regression test (`be/test/storage/version_with_time_test.cpp`): 1 writer publishing 20k versions, 4 spinning readers asserting `ts >= lower_bound[version]` with zero tolerance — it passes deterministically with the fix and has a real failure window under the old order. **2. NGram BloomFilter out-of-bounds read for non-8-byte-multiple sizes** `init()` now does `filter.assign(words, 0)` + `memcpy(filter.data(), buf, size)`, copying exactly `size` bytes; the tail bytes of the last word stay zero so `contains()` still matches query-side filters. Agreed on both meta-points: the over-read is pre-existing (the old per-word loop had it too), and the "reserve()-then-index" root-cause story in the original description was wrong — the constructor already sized the vector. Comments updated accordingly. Added `InitFromExactSizeNonMultipleOfEight` (bf sizes 65/67/100/511/65535) with zero-slack source buffers as the ASan guard. ## Medium issues — fixed **3. USE_AVX2=OFF diverges between BE and CRoaring** croaring now normalizes case-insensitively: `0/OFF/FALSE/NO` disable, `1/ON/TRUE/YES` or empty/unset keep enabled — matching the CMake boolean semantics BE applies to the same raw value. Unknown values print a warning and keep the default. Verified with a 13-value matrix; every documented spelling is now consistent with `if(USE_AVX2)` in `be/CMakeLists.txt`. **4. FileCacheMetrics atomic shared_ptr overhead on the hot path** Replaced with an `AtomicStatistics` value member; the metrics hook is registered in the constructor. This is safe because the counters are fully constructed before the constructor body runs and `instance()` (magic static) publishes the singleton only after construction — so the lazy-init race was indeed unreachable, and the successful-read path no longer pays for a lock-based `atomic<shared_ptr>` load plus refcount churn on every call. Class comments updated to match reality. ## Test & maintainability items - `bitmap_intersect_test.cpp`: comment corrected to describe the int32 coverage; added `RoundtripDateTimeKeysMisaligned` which actually exercises the `Helper<VecDateTimeValue>` specialization (previously untested) from a deliberately misaligned buffer, including the `cast_to_date` branch. - `hash_util_unaligned_test.cpp`: added unaligned-buffer tests for `crc_hash` and `crc_hash64` mirroring the murmur one. - `resolv_shim.c`: added `list(REMOVE_ITEM glibc_compatibility_sources resolv_shim.c)` — no longer compiled into both the archive and the explicit OBJECT library. - `signal_handler.h`: stale "naive method" comment corrected (there is no platform-conditional fallback anymore). - `columns_common.cpp`: the NOTE now accurately describes dropping the `__POPCNT__` gate. You were right that "parenthesized" was wrong — and parenthesizing alone would not even have enabled the block on aarch64, since sse2neon does not define `__SSE2__`. ## Bonus find during verification Linking `doris_be_test` on aarch64 was blocked by a **pre-existing** `-Wundef -Werror` failure in `snii/encoding/crc32c.cpp` (introduced by #66809, unrelated to this PR): the BE_TEST-only TU evaluates `#if SNII_CRC32C_X86` while the macro is only defined on x86. Fixed with `#else #define SNII_CRC32C_X86 0`. This was the single failing target out of 1347 — all review-round sources and all 7 test objects compiled clean. ## Verification aarch64, clang 19.1.7, `-march=armv8-a+crc`: full incremental `ninja doris_be_test` build clean; **16/16 tests pass** across `VersionWithTimeTest`, `NGramBloomFilterTest`, `HashUtilUnalignedTest`, `BitmapIntersectTest`, `SignalHandlerTest`, `ColumnsCommonTest`, `BitPackingUnalignedTest`. PR title and description were updated as suggested: new title, the four inaccurate claims corrected (parenthesized / lock-free fast path / reserve-then-index / never-stale-timestamp), and "Behavior changed" is now **Yes** with the on-disk-format / protocol / hash-output-unchanged clarification. ## Follow-ups deliberately left out of this round - **Narrowing the global `-Wno-error=shadow` exemption** — the existing warnings (protobuf-generated enum collisions, etc.) are numerous; a scoped cleanup deserves its own PR rather than expanding this one's blast radius. - **Regression tests for the bucket-acquire load and histogram `_mm_pause`** — memory-ordering and spin-hint behavior are not practically unit-testable; the comments there are the guard. - **Running the new ngram exact-size test under an ASan build** (`ASAN_UT`) so the over-read guard actually trips on regression — functional semantics are verified in the normal build; the ASan run is scheduled as a follow-up. - **Splitting the PR by theme** — this review round was applied on top of the existing branch to keep the discussion continuous; happy to split into per-topic PRs if maintainers prefer that for review/backport. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
