Repository navigation
[fix](inverted index) Align phrase position defaults on 4.1 - #69190
Conversation
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: FE documents phrase support as enabled by default, but an inverted index without parser properties omitted the setting from its metadata. BE then interpreted the missing setting as disabled. Persist the default for new inverted indexes and align the BE fallback for older metadata. ### Release note Inverted indexes now enable phrase positions by default when support_phrase is omitted. Explicit support_phrase=false remains unchanged. ### Check List (For Author) - Test: IndexTest#testInvertedIndexPersistsDefaultPhraseSupport passed. - Behavior changed: Yes, new inverted indexes with no phrase property write positions. - Does this need documentation: No (cherry picked from commit 1d3838d)
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Derive the omitted phrase setting from the actual analyzer mode in FE and BE. Exact indexes use docs-only postings, while analyzed indexes retain positions. Explicit settings remain authoritative.
airborne12
changed the title
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes: one P1 compatibility defect and two P2 consumer regressions are recorded inline. Historical analyzer-only indexes can be reinterpreted as position-enabled without a rebuild; normalizer-only phrase queries are rejected; TOKENIZE changes its JSON while existing regression outputs remain token-only.
The review converged after two rounds. The FE, BE, and independent risk reviewers all returned NO_NEW_VALUABLE_FINDINGS against the unchanged final comment set. All five changed files and their relevant upstream/downstream paths were covered, every candidate and risk focus was resolved, and the final diff anchors were checked. There were no existing inline threads or additional user-provided focus points; existing_blocking_comment_ids is empty.
The required critical checkpoints were checked individually:
- Goal and proof: newly constructed validated property maps align the FE/BE defaults, but the three inline findings show missed query, SQL-output, and historical-storage consequences. Added helper/constructor assertions do not exercise those consequences.
- Scope: the implementation and unit changes are focused across five files.
- Concurrency: constructor changes use its private copied map; BE fallback reads const properties. No new threads, shared writes, locks, heavy locked work, or lock-order changes.
- Lifecycle and static initialization: no new owning resource, global/static dependency, reference cycle, or release path; existing reader/writer lifetimes remain unchanged.
- Configuration: no process config or dynamic-setting mechanism added. Explicit index-property values still take precedence.
- Compatibility: the P1 historical omission finding is verified against the 3.1.0-rc01/3.1.1-rc01 creation code and current replay/read paths. Existing explicit true/false flags remain authoritative. The two P2 findings cover new normalizer defaults and TOKENIZE output.
- Thrift/protobuf definitions: untouched; no changed field IDs, enum values, or required fields needing release-branch comparison.
- Parallel paths: checked legacy/Nereids DDL, Gson/replay/clone, scan/load/delete/create-replica/alter-index and cloud property transport, scalar/array writers, rebuild/compaction, reader selection, phrase/MultiMatch/scoring, and TOKENIZE.
- Special conditions: aliases and absent/empty maps were traced. Valid parser DDL is restricted to lowercase alternatives, so the apparent case-comparison discrepancy is not a separate supported-input defect. Exact indexes select STRING readers; normalizers select FULLTEXT, which explains the phrase regression.
- Tests: inspected the added FE/BE unit cases and existing consumer regressions. End-to-end normalizer phrase and historical-postings upgrade coverage is missing. No builds or tests were run, as required.
- Expected results: new unit assertions match their direct branches on inspection. Existing TOKENIZE token-only goldens conflict with the newly inferred position output; no regenerated or executed result is claimed.
- Observability: no new distributed workflow needs additional logs/metrics. Existing phrase-setting Status errors expose the normalizer failure.
- Persistence and failover: complete index property maps are already journaled/replayed and sent unchanged. No new journal operation is needed, but Gson/table postprocessing and BE startup preserve historical omission without a mandatory postings migration, supporting the P1 finding.
- Writes, atomicity, and crashes: posting options are chosen before document writes. Existing transaction, publication, cleanup, visible-version, and delete-bitmap boundaries are unchanged. Physical capability reinterpretation is the verified write/read concern; no unobserved crash is asserted.
- FE-BE variables: no new transmitted variable or channel. Existing Thrift/PB maps preserve both explicit flags and omissions across the inspected paths.
- Performance and memory: bounded map lookups add no new document loop, large allocation, ownership, or tracking concern. Posting-size changes follow the selected capability. The existing compaction rebuild/merge hotspot is not solved or claimed solved by this PR.
- Further review: all reviewers and main performed missed-path rechecks, including mandatory migration, selection/fallback guarantees, old validator injection, and shared helper consumers. No other verified finding, unresolved candidate, or uncovered changed-file scope remains.
This was a static review. Runtime queries, tests, and upgrades were not executed; the exact CLucene failure mode for historical missing positions is deliberately not asserted. These execution limits do not leave an unresolved inspection question.
Review converged after 2 round(s).
| return INVERTED_INDEX_PARSER_PHRASE_SUPPORT_NO; | ||
| const auto analyzer = properties.find(INVERTED_INDEX_ANALYZER_NAME_KEY); | ||
| return get_parser_string_from_properties(properties) != INVERTED_INDEX_PARSER_NONE || | ||
| (analyzer != properties.end() && !analyzer->second.empty()) |
There was a problem hiding this comment.
[P1] Preserve legacy docs-only analyzer metadata on upgrade
An omitted flag can already describe a persisted tokenized index with no positions. In the 3.1.0-rc01 and 3.1.1-rc01 implementations, analyzer-only DDL was accepted, but Index injected support_phrase=true only for a parser property; the BE fallback was false and the writer set omitTermFreqAndPositions. Index.read() preserves that omitted property through Gson. After this change, the same unchanged metadata returns true, so the reader advertises phrase/scoring support and multi-term MATCH_PHRASE reaches TermPositionsIterator on files that never stored positions. New writes also switch format under the identical property map. Preserve the old effective false for persisted omissions, or migrate/rebuild those indexes before enabling the new default; a reader-side inference cannot recover their missing positions.
| } | ||
| return INVERTED_INDEX_PARSER_PHRASE_SUPPORT_NO; | ||
| const auto analyzer = properties.find(INVERTED_INDEX_ANALYZER_NAME_KEY); | ||
| return get_parser_string_from_properties(properties) != INVERTED_INDEX_PARSER_NONE || |
There was a problem hiding this comment.
[P2] Account for TOKENIZE's changed output default
This helper also controls the public JSON returned by FunctionTokenize: execute_impl() passes the inferred flag to _do_tokenize(), which adds a position member when it is true. Existing calls that omit support_phrase now return different strings; for example, the Unicode call in regression-test/suites/inverted_index_p0/test_tokenize.groovy:100 has a token-only expected result in regression-test/data/inverted_index_p0/test_tokenize.out:35. The Chinese/ICU/IK cases are affected as well, so their comparisons no longer match. Preserve TOKENIZE's prior opt-in behavior at that consumer, or explicitly adopt the JSON change and regenerate the affected goldens with omitted/true/false coverage.
| || this.properties.containsKey(InvertedIndexUtil.INVERTED_INDEX_PARSER_KEY_ALIAS) | ||
| || this.properties.containsKey(InvertedIndexUtil.INVERTED_INDEX_ANALYZER_NAME_KEY) | ||
| || this.properties.containsKey(InvertedIndexUtil.INVERTED_INDEX_NORMALIZER_NAME_KEY)) { | ||
| if (!InvertedIndexUtil.INVERTED_INDEX_PARSER_NONE.equalsIgnoreCase(getInvertedIndexParser()) |
There was a problem hiding this comment.
[P2] Preserve phrase queries on normalizer-only indexes
For an index declared with PROPERTIES("normalizer"="lowercase"), this now leaves support_phrase absent, so the BE fallback returns false. Such an index still satisfies InvertedIndexAnalyzer::should_analyzer() and gets a FULLTEXT reader; InvertedIndexIterator::read_from_index() then rejects MATCH_PHRASE, MATCH_PHRASE_PREFIX, and MATCH_PHRASE_EDGE. For example, after inserting FOO BAR, WHERE content MATCH_PHRASE 'foo bar' now raises phrase queries require setting support_phrase = true instead of matching. Normalization produces a single whole-value token, and PhraseQuery::add() already executes that case as a term query without positions. Adapt the reader/phrase guard for normalizer-only indexes alongside the docs-only default and cover this end to end.
### What problem does this PR solve? Issue Number: CIR-22263 Related PR: apache#69199 Problem Summary: Inferring phrase positions from a parser when support_phrase is absent changes how older docs-only indexes are read and adds positions to TOKENIZE output. Newly created tokenized indexes already persist support_phrase=true in FE. Keep an absent property false when reading persisted metadata, while allowing normalizer-only single-term indexes to use phrase predicates and similarity scoring without stored positions. Update SHOW INDEX expectations for parser=none. ### Release note Existing indexes with an omitted support_phrase property retain their stored docs-only interpretation. ### Check List (For Author) - Test: Unit Test: focused FE IndexTest and two BE tests passed. - Behavior changed: Yes. Legacy omitted phrase metadata remains docs-only and normalizer-only queries remain available. - Does this need documentation: No
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: This content was flagged for possible cybersecurity risk. If this seems wrong, try rephrasing your request. If you’re doing authorized security work that requires more cyber permissive safeguards, apply for Daybreak access via https://platform.openai.com/settings/organization/status-and-access before retrying. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
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
|
### What problem does this PR solve? Issue Number: None Related PR: apache#69190 Problem Summary: A custom normalizer can split one value into several terms, so omitting phrase positions by default makes indexed phrase queries scan rows. Built-in lowercase normalization emits one term, but its docs-only postings omit frequencies, and collection statistics rejected scoring without norms. Keep positions for custom normalizers, admit unit-length lowercase postings, and use unit frequency when scoring postings without a frequency array. ### Release note Custom normalizers retain phrase positions by default. Built-in lowercase normalizer indexes keep phrase matching and relevance scoring with docs-only postings. ### Check List (For Author) - Test: FE unit tests for normalizer defaults and policy binding; BE unit tests for collection statistics, query scoring, postings, and reader capabilities; C++ format check. - Behavior changed: Yes. Custom normalizers retain positions by default, and docs-only lowercase postings remain eligible for scoring. - Does this need documentation: No
The branch-4.1 target now supports scoring keyword indexes without norms. Keep its postings and WAND behavior, and extend collection statistics to admit built-in lowercase normalizers with one token per row. The upstream keyword postings test covers the behavior duplicated by this branch.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of all 12 changed paths at 13c0431. One verified P2 remains: CREATE INDEX / ALTER ADD INDEX can copy metadata before policy-sensitive defaults are applied, so a retained legacy custom LowerCase normalizer can write docs-only postings despite producing multiple terms. See the inline finding.
All three prior threads were revalidated and are addressed at this head: legacy analyzer omissions still mean false (4294088825), TOKENIZE keeps its omitted-flag output (4294088837), and actual builtin lowercase phrase queries use nonpositional one-term paths (4294088847). No existing P0/P1 blocker remains to carry forward. No additional user review focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: ordinary keyword/lowercase defaults, tokenized/custom defaults and unit scoring are supported by the inspected tests; the ADD INDEX ordering exception remains.
- Scope: the changes are focused on defaults, capability checks and scoring fallback.
- Concurrency: no new threads, shared state or lock order; policy lookup uses the existing read lock.
- Lifecycle: no new static initialization dependency, ownership cycle or cache-handle change.
- Configuration: none added; existing missing-norm controls were traced.
- Compatibility: explicit flags and persisted omissions retain their meanings; the new custom-policy omission is the reported exception.
- Thrift/protobuf: no definitions or IDs changed; release-branch ID comparison is inapplicable.
- Parallel paths: traced CREATE TABLE, CREATE/ALTER ADD INDEX, replay/clone/build, local/cloud transport, MATCH/SEARCH and scalar/batched/range scoring.
- Conditions: genuine builtin lowercase has a single-token guarantee; the lexical shortcut is unsafe on the reported custom binding.
- Test coverage: direct defaults, custom bindings, writer/statistics and missing buffers are covered; ADD INDEX with a retained splitting LowerCase policy is missing.
- Expected results: new assertions and SHOW INDEX strings are consistent; no golden output files changed. Tests were inspected, not executed.
- Observability: existing property display, statistics logs and contextual index errors remain sufficient; no new distributed path needs instrumentation.
- Persistence/failover: existing JSON, edit-log and schema property propagation are unchanged; the reported omission is preserved durably.
- Writes/atomicity: transaction, visible-version, delete-bitmap and crash-recovery control flows are unchanged.
- FE-BE variables: existing property-map send sites cover creation, alteration, scans, sinks and cloud schemas; no new wire variable.
- Performance: added work is bounded property lookup and constant work per scored hit; no new unbounded scan, owning buffer or heavy critical section.
- Further review: policy shadowing, array/VARIANT admission, phrase fast paths, missing norms/frequencies and SEARCH/WAND parallel paths were rechecked. No additional verified issue remains.
Review converged after 2 rounds: all three latest-round reviewers returned NO_NEW_VALUABLE_FINDINGS on the exact frozen comment set. All candidates and both initial risks have explicit dispositions; no unresolved candidate or changed-path coverage gap remains. Validation was static only: no builds, tests, source edits or submodule initialization.
Review converged after 2 round(s).
| || StringUtils.isNotEmpty(this.properties.get( | ||
| InvertedIndexUtil.INVERTED_INDEX_ANALYZER_NAME_KEY)) | ||
| || (StringUtils.isNotEmpty(normalizerName) | ||
| && !"lowercase".equalsIgnoreCase(normalizerName))) { |
There was a problem hiding this comment.
[P2] Apply custom-normalizer defaults before copying ADD INDEX metadata
On an upgraded catalog retaining a custom normalizer named LowerCase with a splitting filter such as word_delimiter, CREATE INDEX ... PROPERTIES("normalizer"="LowerCase") (also ALTER TABLE ... ADD INDEX) now persists an index without support_phrase. CreateIndexOp.validate() constructs this Index after only resolvePolicyNames(); later SchemaChangeHandler.processAddIndex() runs checkColumn() and adds "true" to the separate definition map, but copies only the columns back to alterIndex. The policy-aware default in normalizeInvertedIndexProperties() therefore never reaches the catalog copy. BE dispatches the exact custom policy, so foo-bar emits multiple terms, yet the writer omits positions and is_normalizer_only() permits multi-term phrase queries to request positions that were never written. Resolve the actual binding and apply its default before copying the catalog metadata, or synchronize the validated properties afterward, and cover this ADD INDEX route with a retained mixed-case custom policy.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#69159 Problem Summary: The raw SEARCH score suite requires expression pushdown and inverted-index execution. Fuzzy sessions can disable expression pushdown, leaving SEARCH to row evaluation and causing an internal error. Set both options explicitly before the suite runs. ### Release note None ### Check List (For Author) - Test: The identical suite file passed CloudP0 in apache#69160 (build 1072387); git diff --check passed. CI for this PR is pending. - Behavior changed: No. Test setup only. - Does this need documentation: No.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed all 13 changed files at cc83ce1, including FE creation/persistence, BE postings/phrase/scoring consumers, six unit-test files and both regression scripts. No new inline findings. All three reviewers returned NO_NEW_VALUABLE_FINDINGS in round 1; every candidate and risk was independently verified, deduplicated or dismissed with evidence.
The existing P2 custom-normalizer ADD INDEX issue still applies: the catalog Index copies properties before policy-aware validation adds support_phrase=true to the definition map, and only columns are synchronized afterward. A retained custom LowerCase policy can therefore write docs-only postings while the reader permits multi-term phrases. This remains actionable and is not reposted. CREATE TABLE validates before copying and does not lose that default.
The earlier P1 legacy analyzer omission no longer applies: the BE fallback remains false and Gson preserves omitted metadata. The TOKENIZE output issue is likewise addressed by that unchanged opt-in helper. The built-in lowercase phrase issue is addressed by the reader capability exception and single-term query paths. No existing P0/P1 was confirmed at this head.
Critical checkpoints:
- Goal and tests: new exact/built-in lowercase indexes omit positions; new tokenized/custom normalizer indexes retain positions except the known ADD INDEX collision. Constructor/policy, legacy omission, reader capability, scalar/batch/range scoring and real writer/statistics tests support the intended paths by inspection.
- Scope and clarity: five production files carry focused default/capability/statistics changes; the remaining changes update tests and SEARCH execution settings.
- Concurrency and locks: no new threads or shared mutable BE state; FE policy reads use existing locks. No new lock order or heavy work under locks identified.
- Lifecycle and memory: no new production ownership or static initialization dependencies; existing reader/cache ownership is retained. Test policy-manager restoration and reader/writer lifetimes were checked.
- Configuration: no new configuration. Existing norms/variant-skip settings retain writer behavior; generic multi-term analyzed indexes without norms still fail explicitly.
- Compatibility: explicit flags and generic legacy omissions remain intact. Existing positional files retain their actual norms; docs-only scoring defaults missing statistics. No new symbols, format version, Thrift/protobuf definitions or protocol fields. Mixed-version deployment behavior was not runtime tested.
- Parallel paths: checked CREATE TABLE versus CREATE INDEX/ALTER ADD INDEX, read/clone/build and thrift/PB propagation, MATCH phrase/prefix/edge, scalar/batch/range scoring, SEARCH v2, nullable/variant paths and TopN/WAND. No distinct additional defect verified.
- Conditional behavior: absence is distinguished from explicit false. The lowercase spelling shortcut works for the built-in; its legacy custom-policy collision is the existing P2 above.
- Test coverage and results: updated SHOW INDEX strings match parser=none metadata; parser=standard retains true. No result files changed. Tests do not cover the failing ADD INDEX lifecycle end to end; runtime multi-value ranking coverage is also limited.
- Observability and errors: existing validation, phrase and missing-norm errors remain visible with segment/field context where applicable; no new background operation requires additional metrics.
- Persistence and failover: existing serialized property maps carry the defaults; no EditLog/transaction state or failover protocol changes. The known map-copy defect is accounted for above.
- Writes and atomicity: no new transaction, visible-version, delete-bitmap, crash-recovery or write-atomicity machinery. Writer properties still control postings consistently except the known custom-policy case.
- FE-BE transport: no new variables; existing properties are sent unchanged via thrift/PB, and TOKENIZE continues to consume the BE opt-in helper.
- Performance: a fixed small number of property lookups and per-hit defaults adds no new large allocation or scan. Docs-only postings reduce position storage; this branch still rebuilds indexes during compaction as the PR describes.
No additional user focus was supplied. This was a static review: no builds or tests were run, no source files were modified, and no submodules were initialized. All changed paths and risk questions were covered; no unresolved concrete candidate remains. The retained known P2 and the validation limits above qualify the opinion; convergence does not mean that known issue is fixed.
Review converged after 1 round(s).
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: CIR-22263
Related PR: #69199
Problem Summary: FE persists
support_phrase=truefor new tokenized indexes and leaves exact indexes docs-only. FE and BE retain the docs-only interpretation of an omitted property on older persisted analyzer indexes, preserving their postings andTOKENIZEoutput. Built-inlowercasenormalization emits one term, so phrase predicates can use docs-only postings and BM25 can use unit frequency and document length. Custom normalizers can emit multiple terms, so new indexes retain positions by default, including when a legacy custom policy shadows the built-in name.This is the
branch-4.1adaptation of #69199. It uses the branch's CLucene scoring path and the keyword-index scoring support already merged intobranch-4.1. This branch has no SNII streamed compaction path from #69192, so this PR does not eliminate docs-only rebuilds or the tokenized T2 merge hotspot in CIR-22263.Release note
New exact and built-in lowercase-normalized indexes default to docs-only postings. Custom normalizers retain phrase positions by default. Lowercase-normalized indexes retain phrase matching and relevance scoring without stored positions.
Check List (For Author)
Remote checks on the updated head are pending.