Repository navigation
[feature](variant) Add a values index for VARIANT columns so that full-text MATCH can run on the whole column (the VARIANT root), not only on one subpath. - #69761
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: three P1 wrong-result cases and three P2 issues are reported inline. They cover final logical-path visibility during indexing, derived subpath MATCH coercion, visibility changes during native compaction, root/path analyzer separation during ALTER, analyzer identifier case, and accepted declared-type ingestion. No pre-existing inline findings were present.
Static review covered all 70 authoritative changed paths and their relevant producers/consumers. No builds, tests, benchmarks, source edits or submodule initialization were performed. The code-review skill was applied; its critical checkpoints conclude:
- Goal and proof: the minimal whole-column token-search pipeline is implemented, but the three P1 cases violate MATCH semantics or query/index parity. Unit/regression tests were inspected and do not cover these triggers.
- Scope and reuse: changes are focused on values-index creation, V2 writing/querying and compaction; existing SNII/analyzer/NULL infrastructure is reused. Logical visibility and analyzer-scope reuse need the inline fixes.
- Concurrency and locks: no new worker threads or lock order; new mutable writer/query/planner state is operation-local. Existing cache handles and compaction publication synchronization are retained. No separate concurrency defect confirmed.
- Lifecycle and initialization: owning writers/readers and borrowed leaf buffers survive synchronous consumption; copied schemas prevent shared-schema mutation. No cross-translation-unit static initialization dependency found.
- Configuration: no new runtime configuration or session variable. New index properties use persisted metadata; existing analyzer case/filter settings are bound. No dynamic-setting propagation gap confirmed; the explicit English analyzer-name check needs canonical resolution as reported inline.
- Compatibility: scope/version stamping and token bytes use existing property-map transport; unsupported layouts and old versions are rejected by the new paths. Mixed-version execution was assessed statically, not run.
- Shared definitions: no Thrift/protobuf definition or field-ID changes.
- Parallel paths: ordinary/DOC, root/path, scalar/index, native/rebuild, local/cloud and vertical/nonvertical flows were examined. Inline findings identify missed path-alias, ALTER and compaction-layout cases.
- Conditions and errors: format/parser/keys/phrase/lifecycle restrictions and missing-index handling propagate explicit failures. The native safety condition and analyzer-name check are incomplete as detailed inline; declared Decimal256 schemas can also pass DDL and then fail supported inserts during index construction.
- Test coverage: inspected BE/FE unit tests and all four changed regression suites; coverage includes NULL/empty documents, arrays, promotions, control bytes, analyzer case, native/rebuild modes and metadata sharing. Add the inline reproductions, including analyzer identifier case and values-index insertion of declared Decimal256 paths. Tests were not executed.
- Expected results: changed expected outputs are consistent with their covered cases; quickTest sorting explains lexical ID order. Static inspection cannot establish output-generation provenance.
- Observability: existing SNII counters and new native/rebuild decision/reason logs expose the relevant paths. No additional observability blocker confirmed.
- Persistence and failover: index properties use existing catalog serialization, converters and CREATE journaling/replay. No new transaction protocol or visible-version rule is introduced.
- Writes and atomicity: checked writer/merge failures abort before rowset publication and native sessions retain abort handling. The compaction visibility mismatch can pass existing row/path-count checks.
- FE/BE propagation: analyzer metadata uses existing MATCH fields and index scope/version use existing property maps; the alias finding shows a semantic binding error rather than a missing wire field.
- Performance and memory: metadata aggregation/eligibility reuse and row-major leaf traversal avoid repeated scans; native scratch retains cap/accounting and checked failures. No performance measurement or release-artifact ABI validation was performed.
- Other invariants: no separate committed-transaction visibility, version, delete-bitmap or memory-lifetime defect was established. Test execution and exhaustive runtime/rolling-upgrade validation remain outside this static review.
User focus: no additional focus was supplied. All candidates and initial risks have recorded dispositions in the shared ledger; no unresolved static coverage question remains.
Review converged after 3 round(s).
TPC-H: Total hot run time: 28622 ms |
TPC-DS: Total hot run time: 151220 ms |
ClickBench: Total hot run time: 24.33 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: Values postings included hidden ancestors such as a="hidden" when a.b="visible" wins in the assembled row. Index published materialized and sparse/DOC cells using the same dotted-descendant visibility ordering as reads. Sort sparse map cells by raw dotted keys to match reader merge ordering, including interposed names such as a-. Include materialized-to-sparse layout changes in native merge eligibility. Root rebuild reads retain raw physical paths and empty arrays until the writer chooses its final layout, preventing path-stat loss and stale source postings. Preserve subpath STRING coercion during project/alias substitution before virtual column pushdown, canonicalize built-in analyzer names, and apply independent root/path scopes to ordinary ALTER ADD INDEX. Reject declared Decimal256 values-index schemas before accepting inserts that cannot be assembled. Retain existing no-values-index error-message compatibility. Physical sparse/DOC Map buckets must not inherit parent SQL index flags: otherwise DOC finalization calls an uninitialized Map index builder and aborts the BE. ### Release note Fix VARIANT values-index visibility, derived subpath MATCH semantics, analyzer binding and ordinary path index additions; reject unsupported declared Decimal256 schemas during DDL and avoid DOC bucket index finalization crashes. ### Check List (For Author) - Test: 106 related FE tests passed. ASAN BE build succeeded; 157 related BE tests passed, with 2 disabled NestedGroup tests skipped. Two focused BE reproductions fail on the original source and pass with these fixes. Complete SQL validation and the JSON-control-byte coverage test are recorded in the follow-up test commit. - Behavior changed: Yes; index and scalar paths agree on visible leaves, ambiguous aliases keep ordinary STRING semantics, and invalid schemas fail before loading. - Does this need documentation: No; preserve the values-index contract.
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: Cover visible-leaf indexing across ordinary materialized/sparse and DOC layouts, raw sparse key ordering, materialized-to-sparse compaction and complete DOC writer finalization. Verify that alias/project substitution preserves ordinary object-path STRING semantics while direct root aliases remain native. Exercise analyzer name resolution, independent ALTER index scopes and declared Decimal256 DDL rejection. Compare alias/direct SQL results before generating expectations, so incorrect parity cannot be accepted as a golden. Exercise generic JSON control-byte escaping through the production serialization entry point, covering the eight lines missing from the CI increment report. ### Release note None ### Check List (For Author) - Test: ASAN BE and FE builds via build.sh succeeded. 158 distinct BE tests passed; 2 NestedGroup tests skipped because the module is disabled. 106 FE tests passed. Two focused BE tests fail on the original source. The 7-suite values-index directory passed generation; the final normal comparison passed 11 related SQL suites, including the two CI error-text failures and ordinary Decimal256 storage. clang-format 16, header hygiene, source whitespace and changed-file clang-tidy pass. - Behavior changed: No; tests cover the accompanying fixes. - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed all 80 changed paths at 7eb0b6cebfe21d53ef94b8d1d1cc2f65c14bf1fc by static inspection. Request changes for the remaining instance of existing alias-lineage finding 4400052215. No duplicate or additional inline finding is posted.
The original mergeable-project alias example is repaired, but ordinary subpath values crossing UNION ALL still lose their scalar lineage. With enable_prune_nested_column=false and enable_match_without_inverted_index=true, consider:
SELECT o MATCH_ANY 'hello'
FROM (
SELECT v['obj'] AS o FROM t
UNION ALL
SELECT v['obj'] AS o FROM t
) u;The reduced analyzed plan is:
Project(MatchAny(o#5, 'hello'))
UnionAll(o#5: VARIANT, no subpath or original column)
Project(ElementAt(v#1, 'obj') AS o#2) -> Scan(t)
Project(ElementAt(v#3, 'obj') AS o#4) -> Scan(t)
LogicalSetOperation.buildNewOutputs creates that metadata-free output. ExpressionAnalyzer.visitMatch omits the former STRING cast, and Match.withChildren cannot recover a subpath from the bare union slot. PushProjectThroughUnion accepts only slot/cast chains, and the alternate PushDownProject UNION rule returns unchanged when nested-column pruning is disabled. The MATCH therefore remains above the union; ExpressionTranslator.visitMatch then rejects it as a root with no unambiguous index binding. This is an ordinary subpath query that previously had scalar STRING semantics, separate from the deliberately unsupported UNION of actual root columns. Preserve scalar/subpath lineage through this boundary and cover it in regression tests. This retains the existing issue's identity rather than reposting it.
The other five existing findings are addressed at this head: finalized ordinary/sparse/DOC postings now suppress hidden ancestors; native planning rejects materialized-to-sparse transitions; ordinary ALTER ADD INDEX uses separate root/path analyzer scopes; English analyzer names are canonically resolved; and declared Decimal256 paths, including arrays, are rejected during values-index CREATE validation. Their original examples have targeted tests in the diff. Existing P1s 4400052211 and 4400052218 are not carried forward.
Critical checkpoint conclusions:
- Goal and correctness: the supported root token-search implementation and test coverage are present; the derived-subpath regression above remains.
- Scope and reuse: changes implement the stated minimal feature and reuse SNII text lowering, canonical leaf traversal, storage conversion, NULL tracking and index transport.
- Concurrency and locks: no new worker or lock order is introduced; reader-map/cache lifetimes and compaction publication retain existing synchronization.
- Lifecycle and initialization: root writers own their SNII writer, borrowed leaf bytes survive synchronous document consumption, and finish/error paths release or transfer tracked resources. No new cross-translation-unit initialization dependency was found.
- Configuration: no new server/session configuration is added; existing compaction switches select native or rebuild behavior.
- Compatibility: scope plus format version 4 gates root selection, TOKEN bytes remain
0x07plus token bytes, and DDL rejects unsupported versions/modes. Rolling-upgrade runtime behavior was not executed. - Shared definitions: no thrift/protobuf field or enum changes.
- Parallel paths: CREATE/ALTER/BUILD/DROP, filter/projection, root/path/SEARCH/scoring, ordinary/DOC storage, and full/cumulative/nonvertical/local/cloud compaction were traced. UNION subpath projection remains the verified exception above.
- Conditional checks: restrictions correspond to concrete unsupported query/layout/type combinations; unsafe native proofs choose rebuild rather than trusting postings.
- Test coverage: inspected FE/BE unit tests and SQL suites for visibility, NULL/empty/NOT/OR/count, analyzer case, index/scan parity, conversion, native merge and rebuild. No builds, tests or benchmarks were run in this review.
- Expected results: changed SQL outputs were inspected against their queries; this static review does not independently establish execution or output-generation provenance.
- Observability: native/rebuild decisions include index/tablet and reason; existing reader counters and the existing merge latency timer complement those logs.
- Persistence and failover: new index properties use existing persistence/serialization; no new EditLog or transaction protocol was introduced.
- Data writes and atomicity: schema extension creates a new schema; native merges preserve validated rowid coverage and existing abort-before-publication behavior. Rebuild indexes finalized storage values.
- FE/BE propagation: scope/version and analyzer settings use existing index-property and MatchPredicate channels; no new wire variable requires another send path.
- Performance and memory: source metadata is aggregated once for all roots, row-path processing avoids scanning every path for every row, and native merges have memory preflight plus hard-cap failure handling. No performance result is claimed.
- Final missed-path sweep: all candidates and risk focuses have explicit dispositions; no additional substantiated finding or unreviewed changed-file scope remains.
There was no additional user-provided focus. Review convergence is separate from the blocking verdict: the retained existing finding still requires changes. Validation is limited to static source and test inspection.
Review converged after 3 round(s).
Existing P0/P1 findings confirmed for this head: #69761 (comment)
TPC-H: Total hot run time: 28795 ms |
TPC-DS: Total hot run time: 151420 ms |
ClickBench: Total hot run time: 24.29 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
FE UT Coverage ReportIncrement line coverage |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
|
run cloud_p0 |
|
run external |
|
run vault_p0 |
|
run nonConcurrent |
Add variant_index_scope=values for an inverted index on a VARIANT column. MATCH/MATCH_ANY/MATCH_ALL on a base-table VARIANT root column that has a values index binds to that index; every other VARIANT input keeps the master CAST-to-STRING behavior. DDL validation for values indexes lives in IndexDefinition and CreateTableInfo. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The values index holds the canonical text of every scalar leaf of the final stored VARIANT value, built from the assembled value at segment flush. Compaction merges it natively when the generic SNII eligibility passes and otherwise rebuilds it from the whole root value. MATCH on a VARIANT root without an index evaluates the same leaf-token semantics. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pin three behaviors questioned in review: ordinary path indexes still serve v['s'] MATCH after values-index compaction (native and rebuild, vertical expansion on and off), hidden ancestors stay hidden under WHERE and NOT with and without the index, and a subquery alias of v['obj'] keeps subpath MATCH semantics. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CREATE TABLE accepts index column names case-insensitively and keeps the spelling from the INDEX clause, so a column `V` with `INDEX values_idx(v)` was invisible to the root MATCH lookup. Compare names ignoring case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and BUILD INDEX - Reject BUILD INDEX on a table with a VARIANT values index. Rewriting an SNII container can rebuild every resident logical index with the generic builder, which cannot index VARIANT; that builder now returns an error Status instead of failing a fatal check. - Merge a values index natively only when compaction keeps every indexed leaf type. Type promotion, sparse or DOC input and whole-root rewriting rebuild it, so indexed and index-disabled MATCH agree after compaction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… types Revert the native-merge veto of 3a63ad3 and keep the BUILD INDEX fix. The veto rebuilt every root with sparse or DOC input, so most wide documents lost native merging. The drift it covered needs the same path stored as different types across input rowsets (boolean and number, integers beyond 2^53 and doubles, or FLOAT and DOUBLE) and a MATCH term equal to a promoted leaf's old or new text; it stays a documented limitation, and the type promotion that changes the stored value will be fixed separately. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: Existing values-index tests could miss lost positive MOW postings and promoted numeric leaves, and did not select real horizontal compaction. Add index-only positive predicates, complete-value checks, horizontal/vertical native/rebuild matrices, missing logical index fallback with selective root rebuild, NULL compound predicates and updates, analyzer/template/path-index lifecycle cases, and index metadata persistence coverage. Product behavior is unchanged. ### Release note None ### Check List (For Author) - Test: Regression test / Unit Test - All 8 values-index regression suites pass against ASAN FE/BE product head e263ab5, using generated and reviewed expected outputs. - 2 ASAN BE tests pass, including 16 DUP/MOW, ordinary/DOC, horizontal/vertical and native/rebuild combinations. GLIBC_COMPATIBILITY=OFF avoids duplicate toolchain compatibility symbols in the UT link. - IndexTest: 3 tests pass, including metadata write/read and legacy-scope compatibility. - Repository clang-format 16, build hygiene and clang-tidy checks pass for the modified C++ test lines. FE build and Checkstyle pass. - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: The MOW test combined old, new and deleted terms and did not emit its post-compaction results. Numeric-promotion queries only emitted their pre-compaction results. Check each obsolete term independently, emit positive and negative postings after compaction, and compare complete stored roots and individual numeric terms before and after compaction. ### Release note None ### Check List (For Author) - Test: Regression test; values-index MOW and compaction suites passed in the 14-suite local ASAN regression run; outputs generated by the regression runner. - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: Values-index tests need stronger evidence for coexistence with multiple ordinary and template indexes, index inheritance, compaction planning, and schema changes with historical data. Add six regression suites and expand NULL predicates with runner-generated fixed results. Verify index IDs and column UIDs during ADD, DROP, same-name re-add, RENAME and ordinary-column changes; check failed DDL state and partition-scoped index drops. Exercise deterministic MOW writes while a heavy schema change is blocked, and pin the documented native boolean/numeric promotion behavior separately from rebuild. ASAN fixtures verify physical (index ID, suffix) postings, actual column groups and horizontal paths, missing logical values-index rebuilds, rowid remapping, real empty rowsets, all-NULL/empty documents, zero-output refill and native merge failure/retry followed by another compaction. No product code is changed. ### Release note None ### Check List (For Author) - Test: 14 local regression suites passed without golden regeneration, plus a focused rerun of failed-DDL FE schema-version checks; six ASAN BE tests passed; FE IndexTest passed 3/3; seven tables retained data and index metadata across actual FE/BE restart. clang-format 16, build hygiene and changed-line clang-tidy passed. Cloud branches were not executed. - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: Add data-bearing DUP/MOW controls that verify rejected compound ALTER statements preserve metadata when no Bloom Filter is configured. Compare multiple ordinary and template indexes with and without a values index before and after compaction. Persist explicit analyzer selectors in FE index metadata and verify complete documents and every expected physical index after a second compaction. Keep the confirmed pre-existing Bloom Filter atomicity defect in a separate local reproduction without changing product behavior. ### Release note None ### Check List (For Author) - Test: 16 local regression suites, 6 ASAN BE tests, FE IndexTest 3/3; runner-generated goldens verified without generation; clang-format 16, build hygiene, changed-line clang-tidy and focused Checkstyle passed. - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: Values indexes could only be declared at table creation. Allow CREATE INDEX and ALTER TABLE ADD INDEX through the existing light index change path when enable_add_index_for_new_data is enabled, leaving historical rowsets untouched. Allow full-table DROP through the existing index deletion jobs. Keep BUILD and partition-scoped DROP unsupported. Validate one values index per column independently of path indexes, reject heavy or mixed schema changes, and require SNII for existing and future Cloud partitions. Removing a values index restores the existing root MATCH restrictions; projected MATCH expressions retain the existing STRING coercion. ### Release note Support adding VARIANT values indexes for newly written data and dropping them from existing tables. Historical BUILD remains unsupported. ### Check List (For Author) - Test: 74 FE unit tests, FE build and Checkstyle, and 8 ASAN regression suites passed. Regression expectations were generated by the runner and verified in a normal comparison run. - Behavior changed: Yes. Existing tables can add a values index for new data and drop it. - Does this need documentation: Yes. The values-index DDL documentation should describe the new-data-only requirement and DROP behavior.
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: Cover values-index ADD/DROP on existing tables for DUPLICATE and MOW UNIQUE models with ordinary and DOC storage. Verify historical rowset identity, absence of new BUILD jobs, strict indexed queries on new data, scalar fallback on historical data, MoW updates, compaction, other-index retention, full row preservation, and same-name recreation with a new index id and analyzer settings. Update obsolete rejection expectations and test session handling. Include FE guards for duplicate scopes, disabled new-data mode, mixed changes and Cloud partition formats. ### Release note None ### Check List (For Author) - Test: 74 FE unit tests and 8 ASAN regression suites passed. Generate regression outputs with the runner and verify them without generation flags. - Behavior changed: No. - Does this need documentation: No.
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: Existing storage, compaction and schema-template coverage primarily declares values indexes during CREATE TABLE. Exercise new-data-only ADD and full DROP with horizontal and vertical compaction, native postings merge and raw rebuild, materialized/sparse/DOC layouts and hash bucket counts. Cover DOC-only input becoming materialized after compaction, typed paths moving to sparse storage, multiple analyzer/template indexes, two root values indexes, survivor index ids, atomic duplicate rejection and same-name recreation. Check physical rowset replacement and complete values, and explicitly retain the existing sparse-template path MATCH fallback boundary. ### Release note None ### Check List (For Author) - Test: 13 related ASAN regression suites passed with normal golden comparison; rerun the compaction suite after strengthening the DOC materialization threshold. Generate new outputs with the regression runner. Inspect 18 compaction output footers, DOC input footers, and BE logs to confirm physical layouts, rebuild/native routes and index removal. - Behavior changed: No. - Does this need documentation: No.
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: A Cloud partition-format change can validate before a concurrent new-data-only values-index ADD and then persist a non-SNII format after the ADD succeeds. Recheck the index/format invariant under the existing table write lock immediately before property mutation. Also align ADD/CREATE INDEX's MOR value-column analyzer checks with CREATE TABLE, which already rejects parser aliases, analyzers and normalizers as well as parser properties. ### Release note Prevent concurrent Cloud format changes from violating the values-index SNII requirement and reject analyzed index ADD on MOR value columns consistently with CREATE TABLE. ### Check List (For Author) - Test: Unit Test / Regression test - 111 focused FE unit tests passed, including the deterministic Cloud race. - All 19 values-index regression suites passed on the isolated ASAN runtime. - Behavior changed: Yes. Reject incompatible DDL before metadata mutation. - Does this need documentation: No. Enforce the existing model and format restrictions.
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: Existing tests cover sequential Cloud format validation and parser-based MOR restrictions but miss an index ADD between format validation and mutation, and analyzer-based ADD on MOR tables. Add a deterministic two-thread test for V2/V3 changes that verifies rejection, retained SNII format and index metadata, and lock release. Cover ALTER ADD and standalone CREATE INDEX with parser, analyzer and built_in_analyzer properties in FE and SQL tests. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test - Three new focused FE cases fail on the original implementation and pass with the fix. - 111 focused FE unit tests and all 19 values-index regression suites passed. - Behavior changed: No. - Does this need documentation: No.
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: Cloud ADD values index rejects legacy V2/V3 tables whose existing and future partitions are SNII. RESTORE also loses source partition formats during registration and replay, and its historical signature permits ordinary same-name indexes to enter values-index tables. RECOVER can attach recycled legacy partitions after a light values-index ADD. Use the future partition format for Cloud values-index ADD, preserve source formats in RESTORE metadata and tablet creation, and reject incompatible RESTORE definitions and RECOVER formats before attachment. Reject non-SNII containers before opening the BE values writer. Keep the historical signature and ordinary-index compatibility checks unchanged. ### Release note Fix Cloud VARIANT values-index ADD and preserve SNII partition formats during RESTORE. Reject incompatible RESTORE and RECOVER operations before installing partitions. ### Check List (For Author) - Test: Focused FE unit tests, ASAN BE values-index tests, Checkstyle, clang-format 16, build hygiene and clang-tidy; detailed results in the companion test commit - Behavior changed: Yes, legal Cloud values-index ADD succeeds and incompatible partition restores are rejected - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: Cover legal Cloud ADD and re-ADD on legacy tables, partition format persistence through RESTORE and replay, actual Cloud tablet format fields, RECOVER rejection without consuming the recycle entry, RESTORE definition and physical format compatibility, and the BE writer initialization guard. Add a Cloud Docker lifecycle regression for ADD, DROP and RECOVER. ### Release note None ### Check List (For Author) - Test: build.sh --fe passed; 73 focused FE unit tests and 3 ASAN BE values-index tests passed; Checkstyle, clang-format 16, build hygiene and clang-tidy passed. Cloud Docker regression syntax checked; runtime not run locally because Docker socket access is denied - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: RECOVER and RESTORE (including cloud RESTORE, whose tablet meta is copied from the backup through MS) can bring a V2/V3 partition into a table that has a VARIANT values index, which then reaches the SNII-only values-index writer. 4000def guarded each FE lifecycle path separately, still missed cloud RESTORE and changed FORCE and ordinary path-index RESTORE behavior. Replace those FE checks with one BE rule: a values index is stored only in SNII index files. A segment with a V2/V3 index file is written without the values index and registers no values-index reader, so MATCH evaluates rows, as for segments written before a new-data-only ADD. The FE lifecycle changes from 4000def are reverted. ### Release note None ### Check List (For Author) - Test: BE unit test, regression test - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
### What problem does this PR solve? Issue Number: None Related PR: apache#69761 Problem Summary: Replace the FE lifecycle tests from 5eb1655 with a BE test that writes VARIANT segments with values-index metadata into V2, V3 and SNII index files, with and without a sibling text index: legacy segments keep their data and have no values index, SNII segments keep their postings, and full compaction of both keeps all rows. ### Release note None ### Check List (For Author) - Test: BE unit test (ASAN), FE unit test, regression test (variant_p0/v2/with_index, 21 suites) - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
31a977f to
3f8e117
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: two previously reported P1 correctness findings still apply at head 3f8e117c1e3a8a20e88b5a967519ea4a36718217. No new nonduplicate finding was substantiated. The review converged after one round: both complete reviewers and the separate compaction-risk reviewer returned NO_NEW_VALUABLE_FINDINGS, all 94 changed paths were covered, and every candidate/risk focus has a disposition.
Existing findings retained without duplicate inline comments:
- P1: native merge retains obsolete leaf text.
construct_index_compaction_columns()admits the values index through generic SNII eligibility without checking final VARIANT leaf types;plan_values_index_compaction()then leaves eligible roots on native merge. The new promotion-contract golden explicitly confirms the result: after merging{"n":true}with{"n":1}, stored rows both contain1, but indexed MATCH 'true' still returns row 1 while row evaluation returns none; indexed MATCH '1' returns only row 2 while row evaluation returns both. The author acknowledges the limitation; the current code still exhibits the reported mismatch. - P1: sparse-layout changes invalidate ancestor visibility. The assembler still omits a materialized empty array, while
SparseColumnMergeIteratorserializes non-NULL[]. With the reported top-two-path scenario, movinga.b=[]into sparse storage can hidea="needle", while the native values merge retains the original needle posting. The underlying VARIANT layout transformation predates this feature, but the values-index/scanned-result divergence remains. This is already reported, so it is retained by comment ID rather than reposted.
Critical checkpoint conclusions (static inspection):
- Goal and correctness: whole-root scalar-leaf MATCH, new-data-only ADD, and full DROP are implemented; the two existing compaction mismatches above remain blockers. Shared leaf traversal and final-row assembly address the earlier raw-leaf/hidden-ancestor writer concerns.
- Scope, clarity and reuse: the FE validation/binding and BE storage/query/compaction changes are cohesive for this feature. The implementation reuses VariantAssembler, SNII array documents and existing index lifecycle machinery.
- Concurrency and locks: ADD validation/publication and the cloud future-format invariant use the table mutation lock. The cloud SET/ADD race is repaired and has a deterministic latch test. Operation-owned writer/planner state and existing reader-cache locking introduce no additional substantiated race or lock-order regression.
- Lifecycle and initialization: values writers own their SNII state, retain borrowed batch owners through assembly, and propagate failures through existing cleanup. No new thread, ownership cycle or cross-translation-unit static-initialization dependency was identified.
- Configuration:
variant_index_scopeis a persistent index property. No process/session configuration is added; existing light-index/new-data flags gate ADD. - Compatibility: existing property serialization preserves scope/analyzers; old metadata without scope remains ordinary. V2/V3 segments deliberately omit values writers/readers and use row fallback. No new container codec or serialized column layout is introduced. Mixed-version runtime behavior was not executed; the new feature requires supporting BE code.
- Thrift/Protobuf IDs: not applicable; no gensrc definition changes.
- Parallel paths: CREATE/ALTER/CREATE INDEX, full/partition DROP and BUILD restrictions, cloud/local/ordered/cold/grouped compaction, horizontal/vertical/native/rebuild/retry, MoW updates, and materialized/sparse/DOC paths were traced. Ordinary path/template indexes, SEARCH and subpath scoring remain separated from root values semantics.
- Conditional checks: the format, model, root, missing-index and rebuild branches explain concrete failure scenarios. No additional verified branch defect remained after tracing their callers.
- Tests: inspected all changed FE/BE tests, 19 regression suites and 18 goldens. Coverage includes NULL/NOT/empty tokens, canonical numerics, analyzers, aliases, missing physical/logical indexes, legacy formats, ADD/DROP/recreate, schema changes, MoW and compaction modes. Builds and tests were not run under this static-only contract.
- Expected results: inspected exact golden blocks and indexed/scan comparisons. Results agree with their SQL except the deliberately recorded promotion mismatch, which confirms the existing blocker. Static inspection cannot establish golden-generation provenance.
- Observability: existing eligibility-reason logs, schema/index job history, query/compaction counters and debug points provide tracing; no distinct missing-observability defect was identified.
- Persistence and failover: existing Index serialization and light-schema EditLog/replay carry the complete index metadata. No new transaction protocol, journal record or visible-version mechanism is introduced. Failover was not exercised.
- Writes and atomicity: mixed values-ADD/schema clauses are rejected before live metadata mutation; pure ADD stages copied index lists. Full DROP uses existing deletion-job prechecks. Existing rowset/MoW publication and version rules remain in use; no distinct new publication defect was found.
- FE/BE propagation: scope and analyzer information use existing Index properties and MatchPredicate fields. No new wire field or separate send path was left unhandled in the inspected flow.
- Performance and memory: writer assembly is batched at 4096 rows; native eligibility is decided once and reuses source readers. SNII's tracked build/spill ownership is reused. No additional definite performance or memory-safety regression was substantiated; no benchmark was run.
- Other concerns: the earlier alias, case-insensitive lookup, Decimal256, MOR analyzer, subpath-score, missing-index and legacy-format findings are repaired or unreachable as recorded in the review ledger. Removed RESTORE checks are no longer applicable.
User focus: no additional focus points were supplied. No unresolved candidate or static coverage gap remains. Runtime, cloud deployment, fault-injection and mixed-version execution were not performed; test files and goldens are inspected evidence, not fresh execution results. Review convergence does not mean the two existing P1 bugs are resolved.
Review converged after 1 round(s).
Existing P0/P1 findings confirmed for this head: #69761 (comment), #69761 (comment)
Local pipeline review — ✅ PASSschema: doris-repo-review/v1
status: PASS
pr: apache/doris#69761
commit: 3f8e117c1e3a8a20e88b5a967519ea4a36718217
base: 9e598d38789571a7fc30652aa054f035bd4f1184
reviewed_at: 2026-11-12T12:07+08:00
reviewer: eldenmoon
model: gpt-6-astra
effort: xhigh
findings: {blocker: 0, major: 0, minor: 0, nit: 0}
rounds: 1
converged: trueNotes for maintainers
Reviewed locally with the |
|
run p0 |
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Add a values index for VARIANT columns so that full-text MATCH can run on the whole column (the VARIANT root), not only on one subpath.
Index contents
true/false, integers and decimals in canonical decimal form (42,42.0and42.00all give42), floating values in shortest form. Object keys, JSON null, date/time, binary and UUID leaves produce no text.VariantAssemblerin 4096-row batches after shredding, so typed-path conversion, sparse/DOC storage and hidden ancestors behave the same with and without the index.DDL
variant_index_scope = valueson a VARIANT column, at most one per column, beside ordinary path indexes (their analyzer de-duplication is independent).lower_caseand other defaults follow ordinary analyzed indexes;support_phrasedefaults to false andtrueis rejected.inverted_index_storage_format = SNII. Rejected:field_pattern, NestedGroup VARIANT and declared Decimal256 paths.CREATE INDEXandALTER TABLE ... ADD INDEXsupport existing tables for new data only, withenable_light_index_change=trueandenable_add_index_for_new_data=true. Historical rowsets keep their identity and receive no index build jobs. Ordinary compaction can fill their missing index once the partition has rowsets carrying the new schema. It can be combined only with other ADD INDEX clauses that remain on the light index path.DROP INDEXuses the existing index deletion jobs and retains other root/path/template indexes. Partition-scoped DROP remains unsupported.Query
score()on a root MATCH (subpathscore()is unchanged), aUSING ANALYZERthat does not match the values index, andCAST(v AS STRING) MATCHon the root are rejected. SEARCH() never uses a values index.enable_inverted_index_query=falseuses the same leaf-token semantics onColumnVariantV2.Compaction
trueonce it becomes1). Other promotions keep the leaf's text, such as integer width changes and INT → DOUBLE → STRING. The promotion itself changes the stored value with or without an index ({"n":true}reads back as{"n":1}) and will be fixed separately in VARIANT, as will a materialized[]that reads differently once it moves to sparse storage.Release note
Support a VARIANT values index (
variant_index_scope = values) for full-text MATCH on the whole VARIANT column, new-data-only ADD on existing tables, and full-table DROP.Check List (For Author)
Test
ADD/DROP validation at
1661afce6daeac755beedd317f335b48f57c0459(ASAN runtime; the ADD/DROP change adds no BE code):variant_p0directory with normal golden comparison: 201 suites, 199 initially passed, 2 failed, 0 fatal and 0 skipped. One failure was a stale legacy V1-format golden: the actual full rows matched the independent V2 golden; the runner regenerated the file and a focused normal rerun passed. The remaining OSS outfile case is unverified locally because the default placeholder credentials return403 InvalidAccessKeyId.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)
🤖 Generated with Claude Code