Repository navigation
[fix](index) Enforce SNII compatibility corpus and simplify comments - #68607
Merged
airborne12 merged 6 commits intoOct 26, 2026
Merged
Conversation
airborne12
requested review from
csun5285,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
September 29, 2026 03:29
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
airborne12
force-pushed
the
snii-master-review-cleanup-20260929
branch
from
September 29, 2026 04:55
e9ff8a5 to
277989b
Compare
Member
Author
|
run buildall |
Member
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Complete static review of PR #68607 at head 277989b759100c60d39917d8bdc3f571d1e7f42e: no new findings. There are no existing P0/P1 inline comments to carry forward. The initial main risk scan, two full-coverage reviews, a separate risk-focused review, candidate reconciliation, and final 120-path sweep converged in one round. The live base/head and inline comments were refreshed immediately before submission.
Code-review critical checkpoints:
- Goal and proof: The golden-corpus test now uses eight bundled SNII index/expectation pairs by default. The five analyzed fixtures each cover 27 queries and the three keyword fixtures each cover 12, yielding 513 expected observations across uncached, cache-cold, and cache-warm reads. Static inspection proves fixture presence, query inventory, and verifier wiring; it does not prove a fresh test run on this head.
- Scope and clarity: Effective production C++ and protobuf declarations are unchanged; the large production diff simplifies comments. Functional changes are confined to the golden test's directory fallback, explicit write-mode guard, and expectation license header, plus its fixtures.
- Concurrency and locks: No production thread, lock, atomic, or lock-order behavior changes. The fixture restores the prior global query-cache pointer in TearDown; no new concurrent path was introduced.
- Lifecycle and initialization: No new production ownership or cross-translation-unit static initialization dependency. The test's cache setup and teardown remain paired, including the new early write-mode failure path.
- Configuration: No config value or dynamic-update contract changes.
SNII_GOLDEN_DIRstill overrides the corpus; write mode requires a nonempty explicit directory. - Compatibility: All SNII protobuf fields and tag numbers remain identical, and the supplied binary files retain the existing SNII version 1 container signature. The corpus now supplies a readback compatibility check; no rolling-upgrade protocol or storage writer change occurs in this PR.
- Parallel paths and conditions: The corpus includes scalar and array samples, keyword and analyzed lanes, null bitmaps, phrase behavior, and three cache states. The supported
run-be-ut.shrunner resolves the default path after the build's__FILE__prefix mapping. The write guard and fallback conditions follow their stated behavior. - Coverage and expected results: All eight
.idx/.expectpairs exist, and every expectation has six parseable columns in the same query order as the test source. The verifier compares status, document IDs, null-read status, and null IDs. No build or test was run under this review's no-build instruction, so runtime and exact-head test success remain unverified. - Observability: Existing GTest failures identify missing files, sample open failures, and mismatched sample/query observations; no production logging or metrics path changes.
- Transactions, persistence, writes, and crashes: No production data write, EditLog, transaction, MoW, version-visibility, or crash-recovery path changes. Corpus generation writes files only in explicit write mode.
- FE/BE variables: None added or transmitted.
- Performance and memory: No production path changes. Default unit-test execution adds the finite corpus readback work; the test uses the existing cache fixture.
- Other BE obligations: No new hub-header include, PCH edge, extern-template list, gate-table exception, source-including test, glibc dependency, or new Status/exception path. No additional user review focus was supplied.
This conclusion is based on static inspection of the authoritative bundled diff and checked-in corpus only.
### What problem does this PR solve? Related PR: apache#67657, apache#68607 Problem Summary: The compatibility corpus and comment cleanup conflicted with the posting workspace and spool changes now on master. Merge the current target branch, retain its implementation and interfaces, and reapply concise comments that describe the spool, workspace and merge fan-in behavior. ### Release note None ### Check List (For Author) - Test: Complete C++ formatter scope passed with clang-format 16; BE header hygiene passed. All 57 changed production/protobuf files match master after removing comments. The 16 corpus files are unchanged from the original PR and contain 513 expected observations. Exact-head BE runtime tests have not been run locally; the required Paimon headers and real library are unavailable in the local dependency installation. - Behavior changed: No production behavior changes; preserve the PR's existing default compatibility verification and explicit write directory. - 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: 28085 ms |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
TPC-H: Total hot run time: 27788 ms |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-DS: Total hot run time: 151678 ms |
Contributor
ClickBench: Total hot run time: 24.87 s |
### What problem does this PR solve? Related PR: apache#68607, apache#68758, apache#69192 Problem Summary: The compatibility corpus and comment cleanup conflict with the SNII directory move and query execution changes on master. Merge master, retain its production implementation and deleted query files, and apply concise comments at the new paths. Move the unchanged corpus beside the relocated golden corpus test so default verification finds its input files. ### Release note None ### Check List (For Author) - Test: Static token comparison confirms all 50 changed production/protobuf files match master after comments and whitespace are removed. All 16 corpus files are byte-identical to the previous PR head and retain 171 query records checked across three cache modes. BE header hygiene and full clang-format 16 checks passed. Exact-head compilation and runtime tests remain pending CI. - Behavior changed: No additional production behavior changes. Preserve the existing default compatibility verification and explicit write directory. - Does this need documentation: No.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: five newly bundled analyzed EQUAL_QUERY expectations contain union results, while this head executes the analyzed terms as a conjunction. The committed scalar images yield {0}, and the analyzed array image yields {0,8}. Enabling this test by default therefore exposes deterministic expectation mismatches. One inline finding covers all five copies; no existing inline findings need to be carried forward.
Critical checkpoint conclusions:
- Goal and proof: the corpus is registered and discovered by default, but the stale equality records prevent successful current-head verification.
- Scope and clarity: all 111 changed paths were reviewed. All 50 production/protobuf files have identical non-comment tokens; executable changes are confined to the golden test and one test-name rename.
- Concurrency and locking: no production synchronization changes. The serial test fixture owns its cache; no new lock order or heavy locked operation was found.
- Lifecycle and static initialization: cache restoration, query handles and borrowed readers are balanced; the early write-mode failure precedes cache replacement. No new cross-translation-unit initialization dependency.
- Configuration: no Doris configuration additions or dynamic-config changes. Existing test environment overrides remain supported.
- Symbols, storage formats and rolling upgrades: production behavior and encoded definitions are unchanged. The fixture issue is a query-result expectation mismatch, with no new storage-format regression established.
- Protobuf compatibility: complete definitions match branch-4.2. The file and Snii definitions are absent from branch-3.1/4.0/4.1. No changed/reused ids, enum values or required fields.
- Parallel paths: scalar/array, keyword/analyzed, casing, phrase/docs-only, NULL and three query-cache settings were covered; all five equality instances are grouped in the finding. Unchanged BKD/spill/compaction/count-only paths were checked for comment accuracy.
- Special conditions: requiring an explicit write directory prevents an unintended implicit corpus write; normal runner cwd matches the mapped FILE path. No separate path/setup defect was substantiated.
- Test coverage: eight fixture pairs contain 171 query records for 513 cache-mode comparisons. Existing corruption, differential, memory-accounting and byte-layout assertions remain unchanged. Missing-file/open/parse/mismatch failures remain explicit.
- Expected results: five equality rows disagree with current AND execution and the actual stored postings. Static audits checked binary framing, CRCs, metadata, all 219 term entries/postings and NULL payloads; no second result defect was substantiated.
- Observability: existing expected/actual, sample/open and mismatch-count diagnostics suffice for this test-only behavior change; no new production metrics are needed.
- Transactions, persistence and failover: no EditLog, visible-version, transaction or MoW behavior changes.
- Writes, atomicity and crashes: ordinary verification reads the corpus; generation requires an explicit destination. Production write/cleanup behavior is unchanged.
- FE/BE variables: none added or changed; no transport send path needs updating.
- Performance and memory: production hot paths are unchanged. Verification has a small fixed corpus and one fixture-scoped cache; no new production memory-accounting defect was found.
- Other issues and user focus: no additional user focus was supplied. No further verified finding remained after the final sweep.
Convergence: converged after 2 review rounds, with all reviewers reporting NO_NEW_VALUABLE_FINDINGS on this frozen finding set and no unresolved candidates or unreviewed changed paths. Validation was static only: no Doris build, unit/regression runtime, sanitizer, performance or final ELF validation was performed.
Review converged after 2 round(s).
…ion semantics ### What problem does this PR solve? Related PR: apache#68607 Problem Summary: The bundled corpus contains five multiword EQUAL_QUERY observations with union results. The current FULLTEXT reader lowers analyzed equality to a conjunction, so default corpus verification reports a mismatch in each cache mode. Correct the five expected document-ID fields using the existing index images: the four scalar samples match document 0, and the array sample matches 0 and 8. Preserve every binary index, status field, null bitmap expectation, and other query observation. ### Release note None ### Check List (For Author) - Test: An independent wire-format decoder verified the images' checksums and stored postings. Its equality audit failed on all five analyzed observations before the change and passed all eight corpus equality cases after it. The scope audit confirms eight unchanged index images and 166 unchanged query records. These audits are not a BE runtime test; exact-head runtime results remain pending CI. - Behavior changed: No production behavior changes. Fix test expectations to match the existing analyzed equality contract. - Does this need documentation: No.
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28678 ms |
Contributor
TPC-DS: Total hot run time: 150512 ms |
Contributor
ClickBench: Total hot run time: 24.19 s |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes on head 1b94c07e3669d194a24d2bb781751207bf886a28: two independent P1 defects prevent the newly mandatory compatibility test from passing. Default discovery uses the runner's binary-directory DORIS_HOME as a source root; with an explicit valid corpus directory, nine docs-only phrase/prefix status expectations still disagree with the current reader.
Reviewed all 111 changed paths: 49 BE production files plus the protobuf are comment-only, 44 other BE tests preserve their bodies except one test-name rename, and the remaining scope is the golden test plus eight index/expectation pairs. The static image/query audit examined all 171 records and their NULL fields. Existing finding 4365933499 is fixed: stored equality postings intersect at {0} for the four scalar samples and {0,8} for the array sample. It is not reposted or retained as an existing blocker. The two new findings concern separate discovery and no-position error-code paths. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: making the compatibility corpus mandatory is not yet achieved successfully; the two inline findings identify concrete failures. The three path unit tests omit the actual runner environment.
- Scope and clarity: functional edits are confined to fixture discovery, write-mode protection, and expectation headers; the broad production cleanup changes comments only.
- Concurrency and locks: no new threads, synchronization, lock ordering, or work under locks. The fixture follows existing serial GTest cache replacement.
- Lifecycle and initialization: handles/readers die before fixture cache destruction; the prior cache pointer is restored. New constants are constexpr, with no cross-translation-unit initialization dependency.
- Configuration: no new BE configuration or changed defaults/dynamic behavior. Explicit SNII_GOLDEN_DIR precedence is preserved and write mode requires it.
- Compatibility: production symbols and format logic are unchanged. All eight committed images structurally decode with supported metadata; optional norms/stat fields remain accepted.
- Thrift/protobuf: schema tokens and field/enum IDs are unchanged. The complete branch-4.2 schema matches; the file is absent on branch-3.1, branch-4.0, and branch-4.1. No new required field or reused ID.
- Parallel paths: inspected scalar/array, analyzed/keyword, docs-only/positions, Unicode/lowercase, empty/missing terms, cache modes, and runner launch variants. The docs-only and default-discovery defects are inline.
- Conditional checks: the explicit-directory and write-destination checks have concrete purposes; the DORIS_HOME assumption is incorrect. Production conditions are unchanged.
- Test coverage: eight pairs provide 171 records and 513 intended cache-mode comparisons. Current path tests miss runner integration, and the docs-only negative cases encode stale status codes. Existing test assertions otherwise remain intact.
- Expected results: all recorded docid/NULL sets have static byte/model support; nine english_docs status fields require reconciliation. The previously reported five equality results are corrected.
- Observability: existing missing-file/open/query mismatch diagnostics identify these failures; no new production metrics or logging are needed for the test-only behavior.
- Transactions and persistence: no changed transaction, visible-version, delete-bitmap, EditLog, or persistent schema logic.
- Writes and crash behavior: normal verification reads committed fixtures. Explicit regeneration retains checked writer calls; the new destination requirement protects bundled files. No production write/atomicity/cleanup change.
- FE/BE propagation: no new transmitted variable or sending path.
- Performance and memory: no production hot-path/allocation change. Fixture work is bounded, and its existing cache ownership pattern remains intact.
- Other issues: no further substantiated introduced issue after the complete static recheck.
Validation limits: no builds or Doris tests were run, as required by this review contract. Temporary scripts decoded committed bytes and modeled only the exercised query shapes; they do not replace BE runtime validation. Both findings are independently established by concrete current C++/runner control flow. No remaining unreviewed changed path or unresolved candidate within this static scope.
Review converged after 2 round(s).
Contributor
TPC-H: Total hot run time: 28708 ms |
Contributor
TPC-DS: Total hot run time: 150659 ms |
Contributor
ClickBench: Total hot run time: 24.2 s |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Related PR: apache#68607 Problem Summary: The BE unit-test runner resets DORIS_HOME to the test binary directory. The mandatory SNII corpus test consequently found none of its eight samples in build 1079928. Pass the corpus directory from CMake to the corpus test source and retain explicit absolute and relative directory overrides. Verify that the default directory contains all eight index and expectation pairs. Reconcile the nine docs-only phrase and phrase-prefix observations with the current NOT_IMPLEMENTED_ERROR (3) contract. SniiIndexSource rejects position requests on docs-only indexes, and the shared reader preserves that status. All eight committed index images and the other 162 query records remain unchanged. Merge master 4571c06, retaining the IColumn writer API migration and resolving the introductory comment conflict in the golden-bytes test. Production and protobuf tokens match the updated target branch. ### Release note None ### Check List (For Author) - Test: Reproduced the old helper failure with the runner's DORIS_HOME; compiled the actual path helper, sample list and three GTests using the actual CMake source property; all 18 test executions pass across three working directories with runner and absent DORIS_HOME. BE build hygiene, corpus image/change audits, independent equality posting audit and clang-format 16 passed. Full BE GTest runtime remains for CI. - Behavior changed: No production behavior changes. The compatibility test can locate its bundled corpus in the ordinary BE unit-test runner. - Does this need documentation: No.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed head 9df654a58784172203aab289d4068f2158cb22f0. No new actionable findings.
All three existing P1 findings are fixed at this head: runner corpus discovery, docs-only phrase status codes, and analyzed equality expectations. None remains an existing blocking finding; no duplicate inline comments are posted.
Reviewed all 112 changed paths. Independent lexical audits found no production or protobuf token/preprocessor changes; the other existing tests retain their assertions, with one metadata test renamed. Static decoding validates all eight images, including checksums, metadata/dictionary routing, postings, positions, bloom membership and NULL bitmaps. All 171 stored observations match a query model grounded in the inspected reader, analyzer-selection and lowering paths. The harness compares those observations in three result-cache settings, for 513 comparisons.
| Required checkpoint | Conclusion |
|---|---|
| Goal and proof | Default lookup now uses CMake's absolute source corpus directory; three path tests cover bundled files and absolute/relative overrides. The native corpus test was inspected, not run. |
| Scope and clarity | Behavioral edits are confined to the test harness and its CMake definition; remaining changes shorten comments or rename one test. |
| Concurrency and locks | No production thread, shared-state or lock change. The fixture runs synchronously and restores the previous query-cache pointer. |
| Lifecycle and static initialization | Reader/file/context ownership and fatal-assertion cleanup remain valid; the new license string is constexpr, with no cross-translation-unit dependency. |
| Configuration | No production setting, default or mutability changes. Explicit test-directory overrides are preserved. |
| Symbols and storage compatibility | Production symbols and format logic are unchanged; the committed old images are read as compatibility fixtures. |
| Protobuf compatibility | All definitions retain their tags, labels and enum values; branch-4.2 definitions match. This proto path is absent on branch-4.1, branch-4.0 and branch-3.1. |
| Parallel paths | Scalar/array, keyword/analyzed, docs-only/positioned and result-cache-disabled/cold/warm paths are represented. NULL caching follows its existing independent contract. |
| Conditional checks | Directory selection preserves explicit overrides; write mode fails before mutation when no explicit destination is supplied. |
| Test coverage and negative cases | Fixtures cover empty input, absent terms, ignored long values, control bytes, NULL rows/elements, array phrase boundaries and unsupported positions. Existing assertion bodies are preserved. |
| Expected results | Static byte/semantic audit found zero modeled mismatches across 171 records; corrected equality intersections and the nine status-3 records agree with current code. |
| Observability | Existing failures identify missing files, failed opens, sample/query and expected/actual tuples. No new production path requires metrics. |
| Transactions and persistence | No transaction, EditLog, visibility, delete-bitmap or failover behavior changes. |
| Writes, atomicity and failures | Default verification reads fixtures; writing requires an explicit directory. Production write/error/cleanup logic is unchanged. |
| FE/BE variables | None added; no thrift changes. |
| Performance and memory | No production hot-path or allocation change. Test work is bounded by eight small images and the existing fixture cache, released after restoring the prior pointer. |
| Other issues | Final changed-path, candidate, known-thread and risk sweeps found no additional substantiated defect or unresolved static-review question. |
No additional user focus was supplied. This was a static-only review: no builds, native BE/CLucene tests or sanitizer runs were performed. The analyzer model covers the committed query strings; the binary audit does not establish a native GTest pass.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28636 ms |
Contributor
TPC-H: Total hot run time: 28613 ms |
Contributor
TPC-DS: Total hot run time: 150764 ms |
Contributor
ClickBench: Total hot run time: 25.07 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
gavinchou
approved these changes
Oct 26, 2026
yiguolei
approved these changes
Oct 26, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
4 of 9 tasks
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
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?
Related PR: #67537, #68584, #67657, #68758, #69192
Problem Summary:
The SNII golden corpus test skipped unless
SNII_GOLDEN_DIRwas supplied, leaving existing wire-format compatibility checks out of ordinary BE unit-test runs. This PR bundles eight synthetic index/expectation pairs and verifies them by default across disabled, cold, and warm query-cache modes.CMake supplies the bundled corpus directory to the test source. This lets the test find the files when
run-be-ut.shresetsDORIS_HOMEto the test binary directory.SNII_GOLDEN_DIRstill selects an external corpus or an explicit write destination. Write mode requires that override and adds the Apache license header to generated expectations.The expectations follow the current shared query engine: analyzed multiword equality intersects its terms, and phrase queries requiring positions on a docs-only index return
NOT_IMPLEMENTED_ERROR (3). The original eight index images remain unchanged.The remaining source and protobuf edits simplify comments and rename one metadata test without changing its assertions. A token comparison excluding comments and whitespace confirms that the production and protobuf changes do not alter behavior. The branch includes the upstream IColumn writer API migration.
Release note
None
Check List (For Author)
DORIS_HOME.DORIS_HOME; every run finds all 16 corpus files. This harness does not run the BE query engine.Check List (For Reviewer who merge this PR)