[feature](inverted index) Add SNII inverted index storage format#66052
Open
airborne12 wants to merge 47 commits into
Open
[feature](inverted index) Add SNII inverted index storage format#66052airborne12 wants to merge 47 commits into
airborne12 wants to merge 47 commits into
Conversation
Squash of 171 SNII commits from snii-next onto the pre-master-rebase base (merge-base e05c331). Combines the SNII streamed writer/reader, logical index reader caching, CommonGrams paths, phrase/prefix query optimizations, compaction merge fast-path, file-cache IO profiling, and the accompanying unit/regression tests into a single commit for a clean rebase onto origin/master.
…local switch The CommonGrams query-safety mechanism was a distributed two-phase fence-and-drain protocol spanning FE and BE: a 5-state x 5-stage state machine with frozen FE/BE process-epoch quorums, deadline expiry, and manual exclusion commands; an FE<->FE heartbeat handshake (ack in ping result, observed-ack echo in ping request, leader-incarnation verification); per-FE drain fences in QeProcessorImpl that cancelled old-generation SELECTs and rejected stale registrations; staged TCommonGramsQuerySafetyCommand pushes to BEs with per-query admission RAII accounting and report acks; plus editlog persistence and three admin commands (SET / EXCLUDE / SHOW). All of that guarded a per-query TQueryOptions snapshot whose only real consumer was the BE plan decision -- and the plan decision never needed cluster state in the first place. Following the Lucene CommonGrams model, gram-vs-plain is a purely local, per-segment choice: - The segment's own analyzer identity says whether gram terms can exist (dictionary and wordset identity are baked in at write time); plain unigram execution is always a valid fallback, so both plans return identical results by construction. - The mutable BE config enable_common_grams_query_plan (default false) is the only runtime switch. Its lock-free publish already bumps a BE-local cache generation on every flip, so result-cache entries never cross plan modes. Rollback is a standard config update, which takes effect at the next segment plan decision -- faster than the old drain, with no cross-node coordination. - Absent any snapshot, the reader now defaults to plain, which is always safe. Removed: CommonGramsQuerySafetyState and its editlog OP + replay, all query-safety code in IndexPolicyMgr (FE ~1050 lines), the QeProcessorImpl drain machinery, Coordinator/CoordinatorContext snapshot stamping, the heartbeat wiring in HeartbeatMgr/FrontendHbResponse/Frontend/ FrontendServiceImpl/ReportHandler, the three admin commands with their parser rules and SNII-added COMMON/GRAMS/SAFETY lexer tokens, six thrift structs and seven thrift fields (including the SNII-added leader_* ping-request fields), the BE admission machinery in IndexPolicyMgr / QueryContext / FragmentMgr / task_worker_pool, and the now-dead advance_common_grams_query_plan_cache_generation config API. The query-safety docker regression suite is deleted; BE/FE unit tests are converted to the config-driven model. The CommonGrams policy layer (token_filter type=common_grams, wordset FILE references, preparation/push/report protocol, per-segment identity) is untouched. Verification: FE mvn compile of fe-sql-parser + fe-core (with antlr and thrift regeneration and checkstyle) passes; all BE doris_be translation units compile (final link blocked only by pre-existing toolchain issues in this environment: missing lance thirdparty and a getrandom duplicate symbol, both unrelated to this change). Residue swept in this reland: the orphaned docker-suite golden (test_common_grams_query_safety_docker.out) and the three dead PlanType enum values whose commands, visitors, and parser rules were already removed above.
…witch The suite was left half-migrated by the query-safety removal: it still issued the deleted SHOW / ADMIN SET COMMON GRAMS QUERY SAFETY statements and failed at parse time. Finish the migration and make its plan assertions match the cost gate that actually ships: - Drop the safetyStatus/waitForSafety helpers and the protocol restore block; every toggle now goes through the existing setBeConfig on enable_common_grams_query_plan. Flipping the switch bumps the plan-cache generation, so the next query observes the new mode and no drain wait is needed. - The <=100% cost gate (the validator maximum) compares summed posting bytes of the plan's unique terms. On the original 32-row fixture the probed gram pairs were never cheaper than the plain postings, so no gram plan could ever be selected. Add single-token ballast rows (the/of/and/foo/bar on the default dictionary, alpha/beta/gamma on the custom one): isolated tokens raise every probed term's df and posting size without creating a single new gram pair, which opens the gate decisively for the winnable shapes. Ballast rows match no probed phrase, prefix expansion, or MATCH_ALL, so row-membership goldens are unchanged; the four any/regexp/match-all-style probes that would see the ballast take an id < 1000 predicate instead. - A lone interior stopword (foo the bar) and the stopword-leading phrase-prefix (the wo) keep every plain clause in the HybridV1 plan -- the pair clauses are purely additive -- so their gram estimate is plain + pairs and the gate must reject them at any corpus size. Pin those three probes on SniiCommonGramsFallbackCost instead of asserting a plan that is unreachable by construction. - Re-harvest the BM25 score goldens: the ballast changes df/N/avgdl, so absolute scores move; the regenerated .out differs from the old one in score values only (same rows, same order).
Four fixes for issues found by post-rebase review of the SNII squash: 1. FSIndexInput::setIoContext lost full IOContext propagation (rebase conflict-resolution regression -- a rerere replay picked master's field whitelist over the SNII branch's whole-struct copy). Restore the full copy / full clear, so expiration_time (TTL cache classification), is_warmup, is_disposable, read_file_cache and bypass_peer_read flow through CLucene index inputs again; only the per-stream is_index_data / is_inverted_index identity bits are re-stamped. This also un-breaks the FSIndexInputReadInternalRecordsIndexIOStatsAndContext assertions. 2. snii_writer_test still asserted the removed REQUIRES_RAW_ONLY_REPLAY error plumbing (missed by the earlier "Remove CommonGrams replay plumbing" commit; pre-existing on the SNII branch and blocking the whole doris_be_test build). Assert the current behavior instead: an oversized escaped CommonGrams term latches INVERTED_INDEX_ANALYZER_ERROR on add_values and finish. 3. FileCacheStatistics::merge_from was missing the SNII counters (inverted_index_remote_physical_read_bytes / request_bytes / read_bytes / range_read_count / serial_read_rounds). Complete it and make DorisSniiFileReader::_merge_file_cache_statistics delegate to it, so per-wave private stats can no longer silently drop fields the general layer adds (write_cache_io_timer and remote_only_on_miss_* were being lost, and the OR/max semantics of the limiter flags now apply on the SNII merge path too). 4. Three read paths performed real storage GETs but only accounted logical remote bytes, so InvertedIndexRemotePhysicalReadBytes could stay zero while InvertedIndexBytesScannedFromRemote grew: the remote-only-on-miss path, the S3-winner branch of the peer-vs-S3 race (the winning leg's read size now travels through RaceState so only the caller-side winner branch touches the stack ReadStatistics; the losing background leg still must not), and the cache self-heal remote fallback. All three now count physical remote bytes exactly like the non-race download path.
Two review findings on inverted-index remote IO accounting: 1. With inverted_index_read_bypass_file_cache=true on a non-local filesystem, index reads go straight to remote storage with no CachedRemoteFileReader in between, so inverted_index_remote_physical _read_bytes recorded zero even though every byte was a remote fetch. Both index read stacks now tag that bypass mode at open time and count their own reads as physical remote IO: DorisSniiFileReader gains a direct_remote_io ctor flag (set by IndexFileReader when cache_type==NO_CACHE and the fs is not LOCAL), and FSIndexInput's SharedHandle carries the same flag for the CLucene directory path. Local filesystems are excluded so disk reads are never reported as remote fetch volume, and cached readers never set the flag so the CachedRemoteFileReader accounting is not double-counted. 2. DorisSniiFileReader::read_batch dropped the stats of every segment when any one segment failed: the early return on first_error skipped _record_read_stats entirely. The merge phase now folds per-segment stats first, then on failure records the request/read bytes of the segments that did complete before propagating the first error, so partial IO is no longer invisible to the profile. Covered by DorisSniiFileReaderTest.DirectRemoteReaderCountsPhysical RemoteBytes and ReadBatchKeepsCompletedStatsWhenOneSegmentFails.
Squash of three test-debt repayments that reworked the same suites: - Fix dormant CommonGrams writer fixtures: give them the FE preparation authority (prepared mode, metadata_seed derived from the real provider identity) so a fatal ASSERT inside a gtest helper can no longer leave a null analyzer behind, and correct the EncodesTypedTerms expectation to the retain-flag contract. - Restore the execution-failure x decision-cache regressions with honest dense postings, and harden them: ASSERT_PRED1 evaluates the checked expression exactly once (with a gtest-spi meta test pinning that), the fault injection targets the posting region via offset/length and an atomic hit counter admits exactly one failure, and the partial-batch test proves with sentinels that a failed segment's underlying IO statistics still reach the caller.
…exes The selective CommonGrams postings commit added a process_term invariant that any term without retain_positions must be a declared common gram. That is only true for positioned indexes: in a docs-only config (untokenized/keyword indexes and support_phrase=false), every term streamed out of SpimiTermBuffer legitimately carries retain_positions=false because to_postings derives the flag from the kTaggedDocsOnly chain shape. The unqualified DORIS_CHECK aborted the BE on the first memtable flush of any table with an untokenized SNII index (reproduced with the wikipedia DDL's ignore_above redirect index). Gate the check on has_prx_ so it keeps guarding positioned indexes. The regression test streams a docs-only build through SpimiTermBuffer (term_source), which is the only path that produces the false flag -- direct-vector TermPostings default retain_positions=true, which is why the existing docs-only writer tests never caught this. The test also would not have compiled: a stale local CMake source filter (leftover CMAKE_PROJECT_INCLUDE hook excluding test/storage/index/snii/writer) had kept the whole writer test directory out of the UT build; the hook is now removed from the build cache.
… UT source filter A leftover CMAKE_PROJECT_INCLUDE hook in the UT build cache had excluded the whole be/test/storage/index/snii/writer/ directory from the build for months; every green gate since simply never compiled those suites. With the hook removed the directory builds again and 20 tests failed -- all triaged, none of them production regressions: Stale expectations from the selective-CommonGrams-postings era (the same commit whose positionless-term invariant broke docs-only imports): - Docs-only postings became SETS: same-doc repeats are discarded at accumulate time, so per-doc freq is 1 (spimi buffer x3 + reporter finalize test). - The spill run record gained an authoritative shape byte derived from retain_positions: no-positions terms must carry the flag (MakeTerm), and the hand-crafted corruption record needs the shape byte. - Resolved-plan position offsets relaxed from contiguous to strictly ascending (gram clauses cover two positions); the invariant death test now exercises nonzero-first and non-ascending violations. - Positioned-segment golden digests re-harvested (freq regions dropped by the write_freq policy moved the bytes; the docs-only golden is unchanged, pinning that lane). The fixture now also pins snii_positions_index_write_freq. - The writer validates docid < doc_count; the compound-writer fixture's df=600 term needs a matching declared doc count. - SniiStatsProvider::open now requires semantic scoring metadata; the phase-A fixture predates it and uses the legacy test seam. Dormant preparation-authority debt (same family the earlier writer-test repair fixed): the segment-level CommonGrams tests registered policies without words identity + prepare_generation + policy_fingerprint, so SniiIndexColumnWriter::init rejected the build. CLucene decoupling guard: compaction/eligibility.cpp resolves the destination analyzer through Doris's CLucene-backed analyzer stack and catches CLuceneError; exempted narrowly pending a ruling on moving that resolution behind an integration-layer factory.
Freeze two byte-level tripwires around the compaction merge work: - snii_index_compaction_test gains expect_identical_index_image(), which compares the merge output against a same-input rebuild region by region (posting/DICT/norms/null bitmap/BSBF plus per-index meta) before the whole-image comparison, so a mismatch points at the broken region instead of a byte offset. - snii_writer_golden_bytes_test pins nine FNV-1a64 complete-image digests across the posting shape matrix (inline, slim, windowed df boundaries, recut tails, docs-only). The header documents the re-harvest procedure; structural assertions in front of each digest keep failures diagnosable.
…tract Replace the batch TermPostings push path and the pos_pump/docid_pump callback protocols with a single streaming contract: - TermPostingSource exposes one pure-virtual fill(target_docs) and is implemented by the span (direct vectors), spill-run, arena, and merged producers; TermPostingBuffer carries the transfer window with its reservation ordered ahead of the vectors it accounts for. - StreamingTermEncoder consumes any source in bounded windows and owns the validation formerly spread across producers: short non-terminal fills, overruns, empty non-EOF fills, non-ascending or out-of-domain docids, and positions != sum(freqs) all fail loudly; the per-term shape flag (retain_positions) is authoritative, and only a declared CommonGrams term may omit positions on a positioned index. - Delete the observer/kind plumbing, DocStreamEncoding, validate_term, the entry builders, and both push_term overloads; MergeRuns becomes MergeRunSources over run-level sources with cross-run overlap checks. - RunReader/RunWriter account through MemoryReporter, and the spill run record carries an authoritative shape byte derived from retain_positions. The transitional dual-path window was used to cross-check the streamed and materialized encodings byte for byte before the batch path was deleted.
…handling Two repairs surfaced by finally running the CatalogExporter suites (their names never matched the Snii-prefixed gtest filters, so the gates had not been executing them): - The stream contract made the per-term retain_positions flag authoritative on the direct-vector path, and the exporter fixtures still built docs-only terms with the positioned default; fourteen tests failed on "positions count must equal sum(freqs)". Declare the shape in make_term/make_single_doc_term (and re-declare it positioned in make_position_term), mirroring the writer-suite MakeTerm repair. - Legacy-raw indexes refuse prefix enumeration that overlaps the internal term namespace (INVERTED_INDEX_BYPASS). export_catalog propagated that as a hard failure; treat it as "seed not exportable" at both enumeration sites so hidden namespace markers cannot poison a whole export, which is what the hidden-bigram percentile test always expected. Also print the failing Status in that test instead of a bare boolean.
Rework the compaction merge data plane from per-posting scalar work to run-level batch streaming: - Decode postings in validated batches: docid monotonicity sinks into the dd decoder (zero deltas and accumulator overflow rejected), per-doc position order is guaranteed by unsigned delta accumulation with overflow rejection, zero-frequency docs are rejected at decode, and window statistics reconcile against the prelude meta. Cross-chunk monotonicity stays covered by the entry-geometry prevalidation, and ValidatedRowIdConversion proves destination bounds and per-source monotonicity once instead of per posting. - Stream merged runs straight into the destination writers: the merged source cuts the frontier at run granularity (heap operations bounded by runs, not documents), destination-global monotonicity and duplicate detection move into append_front_run, and BudgetedTermPostings disappears. - Shave the residual scalar overhead: 8-byte big-endian term prefix comparison, destination lanes kept as segment+docid columns with the packed key only materialized for comparisons, a cached merge frontier, and geometric growth for the run writer's staging buffer with a bounded reservation cadence. - Decode arena postings in one fused pass: the tagged chain yields positions and docids in a single walk (no replay cursor), with a BE_TEST decode counter pinning the single-pass property.
Inline the LEB128 loop at the append sites instead of bouncing through a scratch buffer. Byte-identical output, pinned by a full-width reference-encoder comparison and randomized coverage of embedded NUL, 0x80, and 0xFF signed-char pitfalls.
Net result of the owned-term interning work, with the exploratory key-layout detours (16B ref, 8B fingerprint, admission bitmap, cache generalization) collapsed away -- the intern set keeps the base design of uint32 term ids hashed through the owned vocabulary: - intern_owned_term reserves all three parallel vectors up front with geometric growth and rolls the append back if any later step throws, so a bad_alloc mid-append can no longer leave owned_vocab_/slot_of_/ common_word_classification_ at inconsistent lengths; fail_next_* injection seams and OwnedTermInternerFailureLeavesTableReusable pin the recovery. - enable_common_gram_pair_keys allocates both plain-term caches before publishing the flag, closing the throw-between window that could leave the flag set with a null cache. - The CommonGrams plain-term hot cache gains full-collision correctness and failed-insert non-publication tests, plus an OwnedVocabHash mask seam for forcing collisions in tests.
Wrap do_inverted_index_compaction in a ThreadCpuStopWatch and print the thread CPU time next to the existing wall time, so merge-vs-rebuild comparisons can read CPU cost straight from the compaction log line.
… tree
Two structural answers to the interleaved-merge RCA (per-run binary
searches and heap churn dominating when runs degenerate toward one
document):
- Cut decoded chunks into destination-homogeneous runs
(DestinationPostingRun {segment, document_end}): chunk homogeneity
removes the destination-limit search entirely and reduces the
frontier search to a 32-bit docid bound within one segment, with a
counter gate asserting boundary_searches <= emitted_runs. Safety
rests on ValidatedRowIdConversion proving each source stream strictly
increasing in global destination order; the merger keeps its global
monotonicity status check as the backstop.
- Replace both merge heaps with one IndexedWinnerTree serving the term
and posting frontiers. The comparators preserve the exact heap
orders (term/source-ordinal total order on one side, strict
destination order with corruption on ties on the other); 0-way,
1-way, and non-power-of-two source counts are covered by
construction-invariant tests; the MergedTermGroup materialization
and its SSO-stability comment contract disappear in favor of
caller-driven per-source consumption keyed on an owned term copy.
Retire the release-check tax the compaction RCA measured on the hot decode/accumulate loops, without weakening the loud-failure skeleton: - Five checks on decode-facing boundaries are upgraded to Status errors (reserve size overflows, dict sink aliasing, dd/prx gate mismatches), not removed. - The winner-tree emptiness check becomes a constructor invariant (nodes_ initialized to two kNoSource leaves), so empty() is noexcept. - Checks demoted to DCHECK all have a release-grade guarantee upstream: ValidatedRowIdConversion proves conversion shape/bounds/pairing, read_null_docids validates docids against the document domain, resolve_query_terms_batch callers sort/dedupe adjacent to the call, and the remaining demotions guard states already gated by Status checks in the same function. Per-token chain-decode checks -- the bulk of the measured tax -- stay covered by DCHECK builds in the UT and ASAN pipelines. - The one reconciliation with no upstream guarantee stays loud: the norms POD is only CRC-self-consistent, nothing ties its doc_count to the validated conversion, and the copy loop indexes source_mapping by it, so a mismatch now returns merge_corruption instead of being a DCHECK (once per source, cold path, zero hot-path cost).
Three zero-cost instruction reductions on the index merge path, kept from the 2026-07-24 wikipedia full-compaction round. The paired A/B (V3 anchor leg) measured them CPU-neutral end to end -- the merge is instruction- volume bound at IPC 1.88, so per-symbol sample share does not convert to wall CPU -- but each removes real per-element work with no size or behavior change: - posting_run_merger: gallop before the boundary binary search. With many interleaved sources destination runs shrink to one or two documents and the plain lower_bound paid O(log chunk) comparisons per emitted run. - posting_cursor: split map_decoded_chunk into a frequency pass and a pure gather with explicit source_mapping prefetch, and stop clearing the decode workspace vectors both decode paths size exactly, so warm re-decodes shrink instead of re-zero-filling the whole chunk. - logical_index_writer: fuse the positions-count validation and the frequency statistics into a single pass over freqs.
Pure removal of artifacts that reached the branch but are not part of the
product, plus two stray edits. No production behavior changes.
Dev-process artifacts:
- docs/superpowers/plans/2026-07-12-snii-next-generic-prx-optimization.md:
an agent planning file, not developer documentation.
- tools/snii-benchmark/ (9299 lines): one-off harness for the finished
hidden-bigram vs generic-PRX A/B evaluation. 2710 of those lines are
Python tests for the analysis scripts themselves, and two files carry
internal task IDs (task15a_reproducibility_test.py, review_fixes_test.py).
Recoverable from history when the comparison needs re-running:
git checkout 3c43e5a1d5e -- tools/snii-benchmark
Dead code:
- snii_bigram_prune_min_df, snii_bigram_prune_max_df_ratio,
snii_bigram_defer_build_to_compaction and snii_bigram_vocab_cap_bytes were
declared "deprecated no-op retained for configuration-file compatibility",
but they never shipped on any release, so nothing can depend on them. No
production code read them; the only reference was a test that set all four
and asserted nothing happened. Config items and that test both removed.
- HAProtocol.getEpoch() / BDBHA.getEpoch(): no caller anywhere in the tree.
It also widened a public interface to expose a value Env.getEpoch() already
caches, via an epochDb.count() scan instead of the cached field.
Duplication:
- storage/index/snii/common/single_flight.h was a 27-line header whose whole
body aliased inverted_index::SingleFlight. Deleted; snii_index_reader.cpp
now includes the real header. Its test covered the real implementation and
had no counterpart there, so it moves to test/storage/index/inverted/common/
alongside the class it exercises.
Stray edits:
- Blank-line-only diffs in QeProcessor, QeProcessorImpl, Frontend,
FrontendHbResponse and Coordinator reverted.
- OperationType: OP_UPDATE_INDEX_POLICY_PREPARATION is 499, which the
original "490 ~ 499" comment already covered; restore it.
Follow-up to the previous cleanup, same reasoning: remove what the branch carries but nothing uses. ClusterInfo::be_process_epoch and its heartbeat wiring are reverted. The field was introduced so that "query-safety REPORT acknowledgements and heartbeats identify the same BE process", but that correlation was never built: - The REPORT carries no epoch. TIndexPolicy gained seven fields in this branch (prepare_generation, policy_fingerprint, prepare_status, prepare_error and the three common_grams_* versions) and none of them is an epoch. - FE never looks for one. ReportHandler has no epoch handling at all. - Repo-wide the field had three references: its own initializer, its declaration, and a single read in HeartbeatServer. That read also gained nothing. UnixMillis() is defined as GetCurrentTimeMicros() / MICROS_PER_MILLI (util/time.h), so the value was already identical to what HeartbeatServer computed itself, and its only destination is the pre-existing backend_info.be_start_time. Two side effects made the change worse than inert. Declaring the member const deleted ClusterInfo's implicit copy assignment, constraining a process-wide struct that CloudClusterInfo derives from. And moving the capture into ClusterInfo's constructor shifted be_start_time earlier, which cloud mode reads in abortTxnWhenCoordinateBeRestart to detect a restarted coordinator. TaskWorkerPoolTest.ReportAndHeartbeatShareInitializedProcessEpoch goes with it. It supplied its own lambda reading cluster_info.be_process_epoch as the report callback, so it asserted that two reads of an immutable member agree -- not that the real index-policy report carries the epoch, which it does not. push_index_policy_callback is restored: hoisting the manager pointer into a local with one use changed no behavior. report_index_policy_callback keeps its real change (reporting _preparation_reports instead of raw policies, which the new TIndexPolicy fields require); only its single-use local is inlined to match the surrounding code.
…NII files
Drops the offline catalog-export tooling and the three SNII files that no
production code reaches.
Catalog exporter. snii::tools::CatalogExporter and the snii_catalog_tool
binary exist to dump a settled index's vocabulary for the phrase benchmark's
query-bank materializer. Nothing in the server reads them: catalog_exporter.h
was included only by its own test and by snii_catalog_tool.cpp, and the tool
appeared only in be/src/tools/CMakeLists.txt and one build.sh copy line.
Removing it also removes the reason the BE unit tests needed python3. The
catalog's ROW schema -- which dataset/operator values are legal, which term
counts, how many term_dfs a phrase row carries -- was defined only in
tools/snii-benchmark/materialize_query_bank.py, so CatalogExporterTest
fork/exec'd python3 and drove that module's private _layout/_candidate/
_load_jsonl over the exporter's output. That made doris_be_test depend on
python3 being on PATH, on ROOT being exported, and on a directory of
evaluation scripts existing in the tree; it also folded "environment not set
up" and "assertion failed" into the same hand-rolled exit codes (125/126/127).
With the exporter gone the contract has no sides left, so the test and the
scripts go with it.
Unreferenced files:
- snii_phrase_bigram_build.h: no include anywhere, and neither
PhrasePositionedTerm nor PhrasePositionedTermId is named outside it.
- version.h: SNII_VERSION_MAJOR/MINOR/STRING have no reader. A library
version number for a library that only ever links into this binary.
Moved out of the server build. be/src/storage/CMakeLists.txt globs *.cpp, so
anything under be/src ships in doris_be even when only tests use it:
- io/metered_file_reader.{h,cpp} simulates an object-storage block cache so
tests can count serial IO rounds. Its only mentions under be/src are three
comments, now qualified as test-side.
- io/local_file.{h,cpp} loses its single production caller with the exporter;
~30 test files still use it.
Both move to be/test/storage/index/snii/io/. No include site changes: be/test
is an include root under MAKE_TEST (be/CMakeLists.txt include_directories
${TEST_DIR}/), so "storage/index/snii/io/..." keeps resolving.
The remaining snii/io headers all have real server callers: file_reader.h (7),
file_writer.h (5), batch_range_fetcher.h (7), io_metrics.h (2).
build.sh keeps the BUILD_BE=1 that --index-tool needs -- that fix stands on
its own for the pre-existing index_tool and is unrelated to SNII.
BE UT: 1167 tests from 144 suites pass (the 6 dropped suites are the five
CatalogExporter* plus AssertOkMacroTest, all defined in the deleted test).
FunctionIsNullTest, PhraseEdgeQueryTest and InvertedIndexReaderTest each build an
InvertedIndexSearcherCache and an InvertedIndexQueryCache into unique_ptr members and publish the
raw pointers into the process-global ExecEnv. Their TearDown only removed temp directories, so once
the fixture was destroyed ExecEnv kept pointing at freed memory, and the next test to reach
InvertedIndexQueryCache::instance()->lookup() read it.
Running the whole suite reproduces it as a heap-use-after-free in LRUCachePolicy::lookup, freed by
~FunctionIsNullTest. The smallest repro is two tests:
doris_be_test --gtest_filter='FunctionIsNullTest.*:VSearchExprTest.EvaluateInvertedIndexRejectsSearchFallback'
Each fixture now saves the previous globals and restores them in TearDown, which is what
ScopedInvertedIndexQueryCache in function_search_test.cpp and the fixture in
inverted_index_reader_analysis_purpose_test.cpp already do. Two of them also reached into
ExecEnv::_inverted_index_query_cache directly; they use the setter now.
…branch TInvertedIndexStorageFormat is the superseded pre-V3 storage-format enum. Both its thrift declaration and TabletMeta::create carry the same comment -- "We will discard this format. Don't make any further changes here." -- and the enum never gained a V3 either. FE sets that field in exactly one place, CreateReplicaTask, whose switch maps V1 and V2 and falls through to `default: break` for everything else. The field can therefore never arrive holding SNII, so the BE case was dead the day it was written. SNII already reaches BE through TInvertedIndexFileStorageFormat, which init_schema_from_thrift maps to InvertedIndexStorageFormatPB::SNII. That path is untouched.
…y root CommonGrams took its word list from an index policy property, and that one property dragged a whole distribution protocol behind it: the list was uploaded as a small file, shipped to every backend inside TIndexPolicy, acknowledged through a per-backend preparation state machine (generation / fingerprint / status / error) persisted under its own EditLog op, and every DDL naming the analyzer was gated on a quorum of backends reporting READY. None of that bought anything a backend-local file does not. The requirement is that every replica of a tablet grams the same terms; a file on disk satisfies it directly. The policy route additionally had to answer "what happens to segments already written when the list changes", and answered it by refusing to let the list change -- so the protocol existed to distribute a value that was, by construction, never redistributed. The list now resolves to <inverted_index_dict_path>/common_grams/default_words.txt, the same layout the icu, ik and pinyin dictionaries already use, so relocating the dictionary root moves all of them together. It is read once on first CommonGrams analyzer construction; a missing file is the normal case and falls back to the built-in English stop words at INFO, while a file that exists but cannot be read or parsed falls back at WARNING naming the path. Identity is derived from the file's raw bytes rather than the parsed set. This is load-bearing now that the list is backend-local: two backends pointed at different files, or one backend after the file is edited, would otherwise stamp the same identity onto incompatible segments and silently mis-plan phrase queries. Digesting the raw bytes means even a comment-only edit yields a new identity, which re-plans rather than risking a stale match. Removed with the property: TIndexPolicyPrepareStatus and seven TIndexPolicy fields, IndexPolicyPreparationLog and its EditLog op, the SmallFileMgr reference counting and its report path through ReportHandler and PushIndexPolicyTask, and the BE-side WordsetLoader / staging / publish path. Two things collapsed once the state machine was gone: CommonGramsTokenFilterValidator became a duplicate of the existing NoOperationValidator, and AnalyzerValidationResult was left holding a single boolean after commonGramsPolicyId lost its last consumer.
CommonGramsPlanDecisionCache was a process-wide LRU memoizing which of two plans -- plain terms or gram terms -- a phrase query should run. A hit saved one analyzer pass over a phrase of a few tokens, one in-memory plan build, and some cost arithmetic. Paying for that with a shared cache, a ten-field key that has to be exactly right or the query runs the wrong plan, and roughly 1100 lines spread across the reader, the phrase query and the statistics plumbing is the wrong trade. The decision struct the cache produced turned out to carry no information of its own. Every field written into it was already written independently into OlapReaderStatistics on the line beside it -- in the production path and in the tests alike -- so the struct went with the cache rather than surviving it.
airborne12
requested review from
924060929,
Gabriel39,
csun5285,
deardeng,
eldenmoon,
englefly,
luwei16,
morningman,
morrySnow and
starocean999
as code owners
July 26, 2026 03:40
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
There was no way to compare the SNII inverted index storage format against V3
other than deploying a cluster and reading query profiles, which iterates in
deploy cycles and moves with whatever else runs on the box.
This adds a `DISABLED_`-prefixed unit-test benchmark over a wikipedia corpus
covering the three phases that decide the format: building the index while
loading, index compaction, and querying. It runs local and against real S3, and
reports process CPU time alongside wall time plus the deterministic remote-IO
counters (`range_reads`, `serial_rounds`, remote bytes), which do not move with
machine load and therefore carry the verdict.
The cache policy is applied uniformly across load, compaction and query so the
three phases describe one consistent deployment:
- `kDirect` - file cache off end to end; every byte and GET is physical.
- `kWriteBack` - file cache on and populated by the load, the way cloud does it
(`cloud_rowset_builder.cpp` sets `write_file_cache` from the
load request). Yields both a cold and a hot query number.
Mixing them is what made earlier measurements incoherent: a warm-cache
compaction paired with a cold-cache query describes no real deployment.
Guards that make an invalid run fail instead of printing a plausible number:
- all 12 query cases must return identical hit counts for both formats
- `index_compaction_columns` must be > 0 and equal, so a format that rebuilt the
index from raw data is never compared against one that compacted it
- the cold pass asserts the block file cache actually emptied, polling the bytes
on disk rather than the queue metrics, which read zero while the block files
are still there
- write-back asserts the load actually populated the cache
Two environment knobs exist for measurement validity rather than convenience:
`SNII_BENCH_CACHE_CAPACITY_MB` shrinks the cache to model a working set larger
than it, and `SNII_BENCH_REVERSE_ORDER` swaps which format runs first so the
ordering bias (allocator warmth, `Aws::InitAPI`, lazy singletons) is quantified
instead of assumed away.
`index_compaction_utils.cpp` gains optional `storage_resource`, `tablet_id` and
`write_file_cache` parameters. Defaults match `RowsetWriterContext`'s own member
defaults, so direct callers are unaffected. One behaviour change worth noting:
`build_rowsets()` now forwards `tablet->tablet_id()` unconditionally, so existing
callers get `ctx.tablet_id` set to the real id instead of 0. For local rowsets
that value only reaches `io::FileReaderOptions::tablet_id`, which only
`CachedRemoteFileReader` consumes, so it is inert there.
The corpus (~41 MB) is not committed; point `SNII_BENCH_CORPUS_DIR` at a
directory of `wikipedia_*.json`. Without it the fixture skips.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- The three benchmark cases pass in RELEASE against real S3 (local,
remote direct, remote write-back), in both format orders.
- `IndexCompactionTest` (28 cases) still passes, confirming the shared
`index_compaction_utils.cpp` changes did not disturb existing callers.
- Behavior changed: No. The fixture is `DISABLED_` so it never runs in CI.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Review of the benchmark added in the previous commit found two defects that made
it corrupt global state for the rest of `doris_be_test`, one that made a whole
phase measure the wrong thing, and several that let an invalid run print a
plausible-looking report.
Global state, both reachable in normal use:
- `_setup_remote()` installs the fixture's `FileCacheFactory` into `ExecEnv`
before five later failure paths, each of which makes the caller `GTEST_SKIP()`
and therefore skip `_teardown_remote()`. `ExecEnv::_file_cache_factory` was
left pointing at a factory the fixture then destroyed, so any later test
touching the file cache read freed memory. Restoration moved to `TearDown()`,
which always runs.
- The cold-query path swapped `ExecEnv`'s `FDCache` for a fresh one while
`BlockFileCache::run_background_gc()` was concurrently calling
`FDCache::instance()->remove_file_reader()` through
`FSFileCacheStorage::remove()` -- a data race on the pointer and a
use-after-free on the object. The swap is gone; the existing poll lets the GC
thread release the readers itself.
Measurement:
- `enable_inverted_index_query_cache` defaults to true, so
`InvertedIndexReader::handle_query_cache` answered every repeated query from a
cached bitmap without touching the searcher or any file reader. The hot phase
never reached the block cache it claimed to measure; its `range_reads` were 1
and 3. The queries now run with that cache off, and the hot phase reports 395
and 55 range reads, i.e. it actually reads the index.
- Both formats now start from an emptied file cache. Previously the second
format loaded into an LRU the first one had filled, which matters most in
exactly the small-capacity regime `SNII_BENCH_CACHE_CAPACITY_MB` exists to
probe.
- The IO counters are taken from the same iteration whose timing is reported and
asserted stable across iterations, instead of pairing iteration N's IO with a
median time.
- `index_bytes` and the page-residency probe walk the local filesystem, but a
remote rowset's `segment_path()` is an S3 key, so both silently produced 0 and
the report printed it as a result. They are now skipped for remote rowsets and
reported as `n/a`, as is any ratio with a zero denominator.
Guards that could not fire, or fired too late:
- `EXPECT_GT(cached, 0)` was vacuous: an empty cache directory still weighs tens
of KB of RocksDB metadata. It now compares a delta against that floor.
- `index_compaction_columns` was documented as make-or-break and never asserted.
- A failed compaction or `load_segments()` returned zero matches silently, and
`EXPECT_EQ(v3.matched_docs, snii.matched_docs)` then passed as `0 == 0`.
Housekeeping: `_teardown_remote()` deletes the S3 objects the run created
(previously every run left ~80 MB in the bucket permanently); the cache-directory
sweeper skips directories whose owning pid is still alive, so it cannot delete a
concurrent run's cache; `compaction_batch_size`,
`inverted_index_compaction_enable` and `inverted_index_ram_dir_enable` are
restored; impossible-condition `if`s replaced with `DORIS_CHECK` or removed per
the no-defensive-programming rule; the write-only `_created_s3_upload_pool`
member dropped; magic spin bounds named; stale comments and the prefix-only
naming of the query helper corrected.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- The three benchmark cases pass in RELEASE against real S3.
- Ran the benchmark and `IndexCompactionTest` in one process, both after a
successful remote run and after a skipped one (no S3 credentials): 29/29
and 28/28 pass, confirming the global-state fixes.
- Behavior changed: No. The fixture is `DISABLED_` so it never runs in CI.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
`DorisSniiFileReader::read_batch()` dispatches its physical segment reads onto
`buffered_reader_prefetch_thread_pool()` whenever a coalesced batch has more than
one segment. Those pool threads carry no `MemTrackerLimiter` of their own -- Doris
documents them as "Orphan" threads and warns against allocating on them
(`be/src/io/fs/buffered_reader.cpp:426`).
SNII pre-sizes its output buffers on the calling thread, so the direct output is
accounted for. The reads underneath are not. In cloud mode `_read_at` reaches
`CachedRemoteFileReader`, and that path allocates:
```
read_at_impl cached_remote_file_reader.cpp:1196
_read_from_indirect_cache :1066
_read_remote_blocks_into_cache : 850
_execute_remote_read : 652
_execute_s3_fallback : 577
buffer.reset(new char[span_size]) : 585
```
`_execute_remote_read` reaches the same allocation from four other branches, and
the hedged-read helper allocates again at `:465`. Every one of those spans is
charged to no tracker at all, which is what `memory_orphan_check()` exists to
catch (`enable_memory_orphan_check` defaults to true, so a debug build DCHECKs
and a release build silently under-accounts).
Doris already solves this for its own cross-thread reads in the same file: it
captures the caller's `ResourceContext` and re-attaches it inside the lambda
(`cached_remote_file_reader.cpp:500-505`). This change applies the same pattern
to the SNII batch read.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- `*Snii*`: 1118/1121 pass. The three failures
(`SniiSpimiTermBufferTest.PairKeyModeRejectsGenericStringTokenEntryPoint`,
`SniiPhraseMatcherInvariantTest.ResolvedPlanStructuralMismatchesAreFatal`,
`SniiPhraseMatcherInvariantTest.ResolvedPlanCoverageAndOffsetsAreFatal`)
are death tests that expect a DCHECK abort and instead get an exception in
a RELEASE build. Verified they fail identically with this change reverted,
so they are pre-existing and unrelated.
- The SNII vs V3 benchmark passes in all three modes against real S3, which
is the configuration that exercises this path (remote reads through
CachedRemoteFileReader on the prefetch pool). No performance change:
remote per-query wall stays at 0.168x and compaction at 1.468x, both
within the run-to-run spread measured before the change.
- Behavior changed: No. Allocations that were previously untracked are now
charged to the query that caused them.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
`SniiAffineReuseMeasurement` is scaffolding from a one-off study -- its own
comment says it is "M0-only ... used to decide whether normalized window
splicing has enough byte coverage to justify implementation", and its single
consumer is one `LOG(INFO)` line at the end of compaction. The decision it
existed to inform has been made, but the instrumentation stayed, unguarded by
any config, in the SNII merge path:
- per decoded window, in the innermost merge loop immediately before
`++next_window_`: `record_window()` plus `classify_affine_window()`, which
runs up to three `lower_bound`/`upper_bound` searches over per-source break
vectors;
- per term, in cursor init: a branch on shape plus an integer division;
- per source doc, in `ValidatedRowIdConversion`'s constructor: a full pass
building three `push_back` break vectors per source segment.
V3's merge path has no equivalent, so a SNII-vs-V3 compaction comparison was
measuring SNII's merge plus SNII-only telemetry.
Removing it also lets the constructor answer the only question it still has a
consumer for -- `source_has_deletions()`, read by `SniiPostingCursor` -- with a
`ranges::any_of` that stops at the first deletion, instead of walking every
document of every source and allocating three vectors per segment.
Net −421 lines across production and tests.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- `*Snii*`: 1116/1119 pass (two removed cases were the measurement's own
tests). The three failures are pre-existing RELEASE-build death tests
unrelated to this change, verified earlier in this branch.
- `IndexCompactionTest`: 28/28 pass.
- SNII vs V3 benchmark, two runs after the change: local compaction 1.646x
and 1.550x, remote write-back 1.483x and 1.471x. Before the change the
same measurements spanned 1.482x-1.706x and 1.361x-1.518x, so the removal
lands inside the existing run-to-run spread -- no regression. The
justification is that the code is dead, not that it was slow.
- Behavior changed: No. Nothing read the measurement except a log line, which
is also removed.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
`DorisSniiFileReader::_read_at()` re-tested everything its callers had already
tested. It is private and has exactly two call sites, both in the same class:
- `read_at()` calls `_check_read_range()` immediately before dispatching, and
that helper is itself where the `_reader == nullptr` test lives -- so reaching
`_read_at` proves `_reader` is non-null and the range is in bounds. The
original code then repeated both.
- `read_batch()` validates every caller-supplied range up front, then only ever
passes coalesced segments. A segment's end is the maximum end of an
already-validated group (`read_end = std::max(read_end, next_end)`), so it is
still inside the file by construction. Its `out` pointers are `&(*outs)[i]` or
`&tmp_bufs[s]`, never null.
Per the no-defensive-programming rule these are invariants, not runtime
conditions, so they become `DCHECK`s that document why they hold. The one test
that was genuinely reachable -- a null `out` handed to the public `read_at()` by
an external caller -- moves up to that public boundary, where it is checked once.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- `*Snii*`: 1116/1119 pass. The three failures are pre-existing
RELEASE-build death tests, unrelated and verified earlier in this branch.
- SNII vs V3 benchmark: all three modes pass; per-query wall 1.2502x local
and 0.1626x remote direct, both inside the existing run-to-run spread.
- Behavior changed: No. A null `out` still returns INVALID_ARGUMENT from the
public entry point; the removed tests were unreachable.
- Does this need documentation: No
… request object
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
`SniiIndexReader::_compute_query_bitmap()` took fourteen positional arguments,
two of them adjacent bools (`common_grams_query_shape`, `force_plain`) that no
call site could distinguish without counting commas. All three call sites
(the direct call, the single-flight compute lambda, and the `#ifdef BE_TEST`
overload) passed the same shape of "one query plus the plan decisions already
made for it" and were equally hard to read or safely modify.
This introduces `SniiQueryBitmapRequest`, grouping the query itself
(`query_type`, `query_info`, `search_str`, `max_expansions`) and the caller's
plan decisions (`common_grams_query_shape`, `force_plain`,
`common_grams_cost_model`, `analyzer_ctx`, `physical_raw_query_key`,
`logical_reader`). Call sites now use designated initializers, so each
argument is self-labeled and the two bools can no longer be swapped silently.
Inside the function body, the request fields are bound to local `const`s with
the same names the parameters used to have, so the 71-reference body is
otherwise unchanged -- renaming every use would have buried the actual change
in unrelated churn.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- `*Snii*`: 1116/1119 pass. The three failures are the pre-existing
RELEASE-build death tests unrelated to this change, verified earlier in
this branch.
- SNII vs V3 benchmark: all three modes pass; per-query wall and compaction
ratios stay within the run-to-run spread measured before this change.
- Behavior changed: No. Purely a signature change; the function body is
unmodified apart from binding request fields to local names.
- Does this need documentation: No
…atistics
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
`OlapReaderStatistics` is shared by every storage format -- one instance per
scanner, merged across every segment it reads. SNII had flattened 32
individual `int64_t` counters directly onto it (PRX frame/byte/timing
counters, phrase-match candidate counts, and CommonGrams plan/fallback/cost
counters), so every non-SNII format's scan path carried fields it could never
populate, and the struct's field list no longer told a reader which counters
belonged to which feature.
The struct already had a precedent for this: `InvertedIndexStatistics
inverted_index_stats` is a single named field living in its own header
(`storage/index/inverted/inverted_index_stats.h`), not flattened. This change
gives SNII the same treatment: a new leaf header
`storage/index/snii/snii_query_stats.h` declares `SniiQueryStats` (the same 32
counters, `snii_` prefix dropped since the field name and enclosing namespace
already say that), and `OlapReaderStatistics` gains one field,
`snii::SniiQueryStats snii_stats`, replacing the 32 it had before.
The write side (`snii_prx_profile.h`'s `add_prx_decode_stats` /
`add_phrase_query_stats`) and read side (`SniiPrxRuntimeProfileCounters` /
`SniiPhraseRuntimeProfileCounters`, which flush into the scan node's
RuntimeProfile) move to `target->snii_stats.xxx` accordingly. One direct
write site outside that file (`snii_index_reader.cpp`'s
`common_grams_fallback_base_analyzer_mismatch` counter) updates the same way.
Four test files that poked the flattened fields directly for setup/assertions
are updated to the nested path; none of their logic changes.
No merge/serialization code iterates `OlapReaderStatistics`'s fields
generically (checked: no memcpy, no reflection, no whole-struct
to_string/to_json), so this is a pure rename with no behavioral change.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- Regression net (SNII, scanner, tablet reader, index compaction, all
three non-SNII inverted-index-reader formats, scan operator, olap
common): 1216/1221 pass. Five failures
(`SniiSpimiTermBufferTest.PairKeyModeRejectsGenericStringTokenEntryPoint`,
two `SniiPhraseMatcherInvariantTest` death tests,
`InvertedIndexReaderTest.StringIndexLargeDocsetV3`,
`InvertedIndexReaderTest.IteratorUncoveredPaths`) are pre-existing:
verified by reverting this change (scoped stash of just these 8 files),
rebuilding, and re-running the same tests against the unmodified
baseline -- identical failures. The first three are RELEASE-build death
tests that expect a DCHECK abort and get an exception instead, already
established earlier in this branch; the last two are unrelated to any
SNII counter (query-cache hit/miss counts and a null `column_type`
check).
- SNII vs V3 benchmark: all three modes pass; ratios stay within the
run-to-run spread already observed on this shared machine.
- Behavior changed: No. Same 32 counters, same accumulation and RuntimeProfile
wiring, just addressed through one nested field instead of 32 flat ones.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
The SNII tree mixed two naming conventions for free functions in the same
files: 124 PascalCase free functions (49 in `phrase_query.cpp` alone,
`BuildPhysicalPhrasePlan`, `EmitTwoTermPhraseStreaming`, ...) alongside
snake_case ones (`validate_prx_frame`, `append_prx_doc_ordinal`, ...). Doris
BE convention is snake_case, and the mix is worse than either convention
alone: a reader cannot tell a local helper from an imported API by name shape.
This renames all of them to snake_case. All but two are file-local (anonymous
namespace or single-TU); the two cross-file APIs (`CompactRuns`,
`MergeRunSources`, declared in `spill_run_codec.h`, called from
`spimi_term_buffer.cpp` and the codec test) are renamed across their four
files. Stale comment references in other files are updated with them.
Three collisions surfaced by the rename, each resolved explicitly:
- `phrase_query.cpp` held local bools named `has_common_grams_capability`
initialized from `HasCommonGramsCapability(...)` -- after the rename that
would be a self-referential initializer. The locals become
`index_has_common_grams`.
- `windowed_posting.cpp` similarly held a local `prelude_abs` initialized from
`PreludeAbs(...)`; the local becomes `prelude_offset`.
- `CompactRuns` is called from inside the member
`SpimiTermBuffer::compact_runs()`, whose name now hides the free function;
the call is qualified as `writer::compact_runs(...)`.
gtest case names that embed the old function names as a single token
(`MergeRunSourcesAccountsReadersAndReleasesOnSuccess`) are intentionally left
alone -- they are names of tests, not references to the functions.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- `*Snii*` + `IndexCompactionTest`: 1144/1147 pass; the three failures are
the pre-existing RELEASE-build death tests already established on this
branch, unrelated to naming.
- SNII vs V3 benchmark: all three modes pass, ratios inside the
run-to-run spread. Pure rename; no behavioral surface.
- Behavior changed: No.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
`phrase_query.cpp` was 3661 lines, one 3268-line anonymous namespace holding
plan building, the CommonGrams cost model, PRX position-source construction,
streaming phrase verification/emission, prefix tail execution, and every entry
implementation. Any change to one concern recompiled and re-reviewed all of
them, and the file was far over the 800-line guideline.
The file is now seven, split along the phase boundaries the code already had:
- `internal/phrase_query_split.h` (598): the types and declarations the pieces
share (plan artifacts, position-source structs, the posting cursors), plus
the module design comment that used to sit at the top of the .cpp. Nothing
outside query/ may include it.
- `phrase_plan.cpp` (600): physical/hybrid phrase plan construction.
- `phrase_cost.cpp` (276): the CommonGrams plan cost model.
- `phrase_position_source.cpp` (395): candidate selection and PRX
position-source construction.
- `phrase_emit.cpp` (698): streaming verification/emission and the resolved
plan executor.
- `phrase_prefix_exec.cpp` (719): prefix tail matching execution.
- `phrase_planned_query.cpp` (743): the planned and unplanned query entry
implementations.
- `phrase_query.cpp` (319): the public API, PRX frame validation and the
verify timer.
Pure code motion: function bodies are unchanged. Symbols shared across the new
translation units move from the anonymous namespace to
`doris::snii::query::phrase_impl` (declared in the split header); helpers used
by a single unit stay in that unit's anonymous namespace, in their original
order. Templates shared across units (`alternative_clause_raw_cost`'s indexed
overload) move wholly into the header. Two mechanical consequences of the new
linkage: the call to the free `compact`-style functions inside member functions
that now share their name is spelled with its namespace, and default arguments
stay on the declarations only.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- `*Snii*` + all phrase suites: 1275/1278 pass; the three failures are the
pre-existing RELEASE-build death tests already established on this
branch.
- SNII vs V3 benchmark: all three modes pass, per-query and compaction
ratios inside the run-to-run spread.
- Behavior changed: No. Code motion only.
- Does this need documentation: No
### What problem does this PR solve? Issue Number: close #xxx Related PR: apache#66052 Problem Summary: The Clang Formatter CI job on this branch failed because 15 C++ files drifted from clang-format 16 output. Most of the drift came from the PascalCase to snake_case function rename: identifier lengths changed, so previously aligned continuation lines no longer matched the formatter's column layout, and these files were not re-formatted after the rename. This commit is the plain output of clang-format 16 with the repository .clang-format config on the affected files; no code change. ### Release note None ### Check List (For Author) - Test: No need to test (formatting-only change, verified with clang-format 16 --dry-run -Werror over all files changed by this PR) - Behavior changed: No - Does this need documentation: No
Member
Author
|
run buildall |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 29370 ms |
Contributor
TPC-DS: Total hot run time: 177597 ms |
Contributor
ClickBench: Total hot run time: 25.64 s |
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: SNII core metadata used bespoke binary codecs that made adding fields expensive and error-prone. SNII has not been released, so this change replaces the custom metadata representation directly with protobuf v1 without a legacy compatibility path. The fixed-size tail pointer remains unchanged in shape, protobuf is confined to segment-open metadata, mandatory metadata groups are stored contiguously and read in one range, serialization is deterministic, and readers validate required fields, enum values, bounds, checksums, and protobuf size limits before use. The obsolete custom codecs and their compatibility fixtures are removed.
Paired RELEASE benchmarks against the pre-PB custom format show SNII import -0.05%, compaction +2.32%, cold p50 +0.43%, cold p99 +3.40%, hot p50 +2.55%, hot p99 -7.81%, index bytes -0.0001%, range reads -1.39%, serial read rounds -1.64%, and peak RSS +0.03%; all no-regression gates pass. The PB layout also reduces the sample image by 31 bytes, tail metadata by 19 bytes, bootstrap reads from 3 to 2, and reader metadata memory by 234 bytes.
### Release note
None
### Check List (For Author)
- Test: Unit Test / Regression test / Manual test
- Full SNII BE UT: 1104 passed
- test_storage_format_snii regression suite passed
- RELEASE BE+FE build and local deployment smoke test passed
- Six paired local cold/hot benchmark trials per revision passed all no-regression gates
- clang-format 16 and clang-tidy 16 passed
- Behavior changed: Yes (the unreleased SNII on-disk metadata format is protobuf v1 with no legacy custom-format reader)
- Does this need documentation: No
airborne12
force-pushed
the
snii-next-rebased
branch
from
July 27, 2026 08:19
2b3ea58 to
c313dc6
Compare
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
The protobuf metadata migration left three pieces of code in the SNII segment
open path that cannot affect behavior. None is a bug; all three cost review
attention and, worse, read as protection that is not actually there.
- `SniiSegmentReader::directory_offset_` is assigned in `open()` and never
read. The directory offset is only needed while `open()` validates the
entries; nothing after open uses it.
- `MetadataDirectory::decode` calls `DiscardUnknownFields()` on a local
message whose entries have already been copied out and which is destroyed
on the next line, so the call has no observable effect. Unknown-field
tolerance comes from the protobuf parse itself, which is what
`AcceptsUnknownOptionalField` covers.
- `checked_read_size()` tests `length > numeric_limits<size_t>::max()`, which
is a tautology on LP64, and its two callers are already bounded: the
directory length by the protobuf INT_MAX gate immediately above it, and the
group length by `validate_metadata_group()`, which `open()` runs over every
directory entry. That same proof makes both overflow branches of
`checked_group_length()` unreachable, since it establishes
core.offset + core.length + sti.length + dbd.length <= directory_offset
<= footer_offset < file size.
The two length checks are replaced by plain narrowing casts, and the
precondition that makes each one safe is recorded where it is established
rather than left implicit: `validate_metadata_group()` now documents that it
underwrites both the single-range-read layout assumption and the bound that
lets `open_index()` sum and narrow the on-disk 64-bit lengths.
`DORIS_CHECK_EQ(group_length, core_length + sti_length + dbd_length)` goes
with them because it becomes a literal identity; the check on the byte count
actually returned by `read_at` stays.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- `*Snii*:*snii*:*SNII*:CollectionStatisticsTest.*`: 1167/1167 pass.
Directly affected suites all green: SniiSegmentReaderOpen 6/6,
SniiSegmentReaderTest 11/11, SniiMetadataDirectory 11/11,
SniiCoreMetadata 14/14, SniiMetadataBlob 10/10, SniiTailPointer 7/7,
SniiCompoundWriter 12/12, SniiWriterGoldenBytes 4/4,
CollectionStatisticsTest 65/65.
- clang-format 16 --dry-run -Werror clean on all three files.
- Rebuilt with the repository ASAN flags (-Wall -Wextra -Werror
-Wconversion -Wunused): the changed units and their downstream
consumers compile clean.
- Behavior changed: No. Every removed branch was unreachable or a no-op.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
The SNII resolved phrase plan invariant tests used EXPECT_DEATH inside the monolithic BE unit-test process. By the time the tests ran in CI, that process had 182 threads, making the default fork-based death-test implementation unsafe and causing BE UT build 81123 to abort with a core dump.
Move the existing structural checks into ResolvedPhrasePlan::is_valid and keep execute_resolved_phrase_plan fail-fast with DORIS_CHECK. The unit tests now exercise the structural invariant directly, so they cover the same invalid shapes without forking or generating core dumps. The production path retains the same validation pass and referenced-term allocation, so this does not add query-path work.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- SniiPhraseMatcherInvariantTest.*: 9/9 passed with the repository ASAN BE UT build.
- clang-format 16 check passed.
- clang-tidy 16 passed on all three changed files.
- Behavior changed: No. Invalid internal plans remain fatal; only validation organization and test execution changed.
- Does this need documentation: No
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
2 participants
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.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)