WIP: Implement lazy posting list apply mode for ProjectionIndex - #3
Closed
fastio wants to merge 1 commit into
Closed
WIP: Implement lazy posting list apply mode for ProjectionIndex#3fastio wants to merge 1 commit into
fastio wants to merge 1 commit into
Conversation
fastio
marked this pull request as draft
February 13, 2026 00:47
amosbird
pushed a commit
that referenced
this pull request
Apr 1, 2026
## #1: Cross-test ownership ratio (find_tests.py) Replace plain `1/width` scoring with `1/(width × region_test_count)` where `region_test_count = uniqExact(test_name)` for each region. A region covered by 1 test scores 1000× higher than one covered by 1000 tests, naturally eliminating the hot-region problem without a hard cap. ## #2: Branch direction (LLVMCoverageMapping + coverage_log) Parse `BranchRegion` (LLVM kind=4) in `readLLVMCoverageMapping`. Format: kind-marker → true_counter → false_counter → line/col, producing two `CoverageRegion` entries with `is_branch=true` and `is_true_branch` set. Added `branch_flags Array(UInt8)` to `system.coverage_log` (0=code, 1=true, 2=false). Tests that covered both sides of a changed condition now carry that information for smarter ranking. ## #3: Indirect call targets (coverage.h/cpp + CoverageCollection) Read `LLVMProfileData::Values` (LLVM value profiling, kind IPVK_IndirectCallTarget) to capture which concrete functions were called via virtual dispatch or function pointers. Each observation: `(caller_name_hash, caller_func_hash, callee_offset, call_count)` where `callee_offset = callee_abs − binary_load_base` (from `/proc/self/maps`), stable across ASLR restarts. Flushed alongside coverage counters into `system.coverage_indirect_calls`. Fixes the fundamental gap where a test covering `IStorage::read` cannot be distinguished from a test specifically dispatching to `StorageMergeTree::read`. ## #4: XRay call-depth tracking (coverage.h/cpp + cmake/sanitize.cmake) New CMake option `WITH_COVERAGE_XRAY=ON` enables `CLICKHOUSE_XRAY_INSTRUMENT_COVERAGE`. At test start, activates XRay (`__xray_patch`) with a handler that maintains per-thread relative call depth. On first activation, builds a `(xray_function_id → profile_data_index)` map by resolving each function via `__xray_function_address(id)` + `dladdr()` → symbol name → FNV64 hash → match against `LLVMProfileData::NameRef`. This solves the PIE `FunctionPointer=0` limitation: XRay provides real runtime text addresses where `LLVMProfileData::FunctionPointer` is always null. When enabled, `min_depth` in `CovCounter` contains actual call depth; otherwise falls back to call-count proxy. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
amosbird
pushed a commit
that referenced
this pull request
Apr 27, 2026
…079-3df0) `LimitSeekableReadBuffer::seek` used the expression `pos + position_change` inside its in-buffer fast-path check. When the inner buffer has not materialized its memory yet — as is the case for `EmptyReadBuffer`, `ParallelReadBuffer`, and `ReadWriteBufferFromHTTP`, all of which start with `pos == nullptr` — and a non-zero seek is requested before the first `next()`, this triggers undefined behavior per [expr.add]: "applying non-zero offset N to null pointer". On master BuzzHouse (arm_asan_ubsan) this surfaced from the BACKUP write path as STID 3079-3df0: ``` src/IO/LimitSeekableReadBuffer.cpp:97:34: runtime error: applying non-zero offset 1178 to null pointer #0 DB::LimitSeekableReadBuffer::seek(long, int) #1 DB::BackupWriterDefault::copyDataToFile(...) #2 DB::BackupImpl::writeFile(...) #3 DB::BackupsWorker::writeBackupEntries(...) ``` Replace the pointer-arithmetic comparison with an equivalent check expressed via pointer *differences* and integer arithmetic, which is well-defined regardless of whether `pos` is null. The condition is mathematically identical to the original for all legal inputs. Add `gtest_limit_seekable_read_buffer.cpp` covering the null-`pos` regression plus two sanity tests for the in-buffer and out-of-buffer seek paths. Without the fix the first test reproduces the exact UBSan error from CI; with the fix all three pass cleanly under `-fsanitize=address,undefined`. CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?REF=master&sha=4b8132993ce0625e9c0ed0bf15ed16eb6df8262b&name_0=MasterCI&name_1=BuzzHouse+%28arm_asan_ubsan%29
amosbird
added a commit
that referenced
this pull request
May 19, 2026
…ax indices Address PR ClickHouse#101170 review feedback (CI #2): - Drop the 5 add_minmax_index_for_* members from ProjectionDescription. They were stale ad-hoc fields duplicating MergeTreeSettings; with the new generic SettingsChanges flow the effective MergeTreeSettings is already the canonical representation. - Thread the already-computed effective MergeTreeSettings (defaults from the projection index plus user WITH SETTINGS overrides) into fillProjectionDescription and fillProjectionDescriptionByQuery as a new parameter, instead of reconstructing it. fillProjectionDescriptionByQuery reads the implicit-minmax flags via strongly-typed MergeTreeSetting accessors. - Add the 5 add_minmax_index_for_* names to ALLOWED_PROJECTION_SETTINGS so the feature added in ClickHouse#105137 (Enable Implicit Skip Indices for Projections) keeps working under the new settings flow. The ATTACH validation gap raised by review item #3 is intentionally left as `mode <= LoadingStrictnessLevel::CREATE` to match the project-wide convention (InterpreterCreateQuery, registerStorageMergeTree, etc. all validate user input on CREATE/SECONDARY_CREATE only). The previous loadSettings validated unconditionally because it had no access to mode; now that we do, aligning with the rest of the codebase is the consistent choice.
alexey-milovidov
pushed a commit
that referenced
this pull request
May 24, 2026
The point-lookup queries (#4-ClickHouse#7) in apply_patch_parts_join produced 18-30% test noise because the four UPDATE statements that build the patch parts ran with default parallelism. Parallel threads racing into the patch builder produce different patch-part split boundaries on every run, which changes how many patch granules a point lookup at id=5000000 ends up reading. Per-server timing then depends on the random split, not on the code being measured. Force INSERT and UPDATE to use a single thread plus a block size large enough to fit each whole patch, so the patch-part layout is the same on every run. Locally verified deterministic over 20 cycles. Setup time goes from ~4.2s to ~6.3s. Scan queries (#0-#3) still detect patch-apply-path regressions as before. Refs ClickHouse#100759.
alexey-milovidov
pushed a commit
that referenced
this pull request
Jun 6, 2026
ClickHouse#106278 tried to remove the LIMIT-without-ORDER-BY non-determinism by sampling with WHERE cityHash64(WatchID) % N = 0. Post-merge master data showed it did not work: it made the measured queries scan the whole table on every run, so instead of reducing the noise it regressed it badly: classification #0: 6.5% -> 76% noisy classification #1: 10% -> 90% noisy (#2/#3/#4 were already clean.) Materialize the deterministic subset once into its own single-part table (single-threaded insert + OPTIMIZE FINAL) and measure over that, so the determinism lives in the setup (like ClickHouse#106249 for reading_from_file) and the measured queries read only the subset instead of rescanning the source.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add a new text_index_posting_list_apply_mode setting that introduces a lazy cursor-based posting list evaluation path for ProjectionIndex queries. The existing materialize mode eagerly decodes entire posting lists into Roaring Bitmaps; the new lazy mode decodes TurboPFor-compressed blocks on-demand, reducing memory usage and improving performance for selective queries.
Detailed description / Documentation draft:
In the current materialize mode, every posting list is fully decompressed into a Roaring Bitmap before set operations. For large posting lists or highly selective queries, this is wasteful — most decoded doc IDs are never examined. A lazy cursor can skip entire 128-doc blocks and stop early once the result is determined.