[feat](inverted-index) Add IK tokenizers to custom analyzers - #67918
Open
airborne12 wants to merge 17 commits into
Open
airborne12 wants to merge 17 commits into
airborne12 wants to merge 17 commits into
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
Static review of exact head 1ecadcaebb31e4bdfc8139194e9434eb044cdea1. Requesting changes for 1 P0, 6 P1, and 1 P2 findings.
Critical checkpoint conclusions
- Goal and proof: The goal is to expose
ik_smartandik_max_wordas custom-analyzer tokenizers. The lowercase scalar happy path is wired, but the P0 reset regression and the policy, offset, and phrase failures mean the implementation does not yet satisfy the supported public paths. The added tests prove selected terms only. - Scope/focus: The production change is small and focused, but its integration surface is broader than the tests cover. No user-specific focus was provided, so the complete PR was reviewed.
- Concurrency: Factory and dictionary publication use
call_once; all production IK initializers use the same dictionary path, per-tokenizer mode remains local, and loaded tries are read-only. No new race, lock-order, deadlock, or heavy-under-lock issue was found. - Lifecycle/static initialization: Dictionary initialization is safe, but tokenizer reset lifecycle is not: CLucene's mandatory reset-before-consume crashes legacy IK indexing and destructively empties custom ARRAY token streams. Reader ownership and retained token storage are otherwise sound on the traced paths.
- Configuration: No new Doris runtime configuration item is added. Existing dictionary configuration remains restart-only and is used consistently.
- Compatibility: No wire/storage-format symbol is added, and the documented BE-then-FE rolling sequence covers factory availability. However, previously legal persisted policies named
ik_smart/ik_max_wordnow collide, with FE and BE choosing different meanings. - Parallel paths: Scalar custom reuse works, but legacy and ARRAY CLucene paths fail reset; both legacy CLucene and SNII phrase readers mishandle same-position pinyin alternatives, while the query-v2 CLucene path groups them.
- Conditional logic: No additional unsupported special condition was found. The missing FE validator cases and inconsistent normalization are called out inline.
- Test coverage: Missing cases include legacy IndexWriter, custom ARRAY, named TOKENIZER policies, mixed-case/noncanonical references, replay collisions, exact offsets, multibyte refill boundaries, and
MATCH_PHRASEacross storage implementations. - Test results: BE/FE tests and a regression suite were added, but their assertions do not cover the failing paths; the regression also violates required golden-output/table-name conventions. Per the review contract, I did not run builds or tests, so all conclusions are static and author/CI claims are not independent execution evidence.
- Observability: No new metrics or logging are required for this registry exposure; existing exception boundaries are adequate once the correctness defects are fixed. The ARRAY failure is silent data omission and must be corrected rather than observed around.
- Persistence/transactions: No transaction state machine changes. Existing policy journaling/replay is structurally used, but replayed-name collision semantics are incompatible as noted inline.
- Data writes: Index construction is materially affected: existing legacy IK writes can crash and custom IK ARRAY values can contribute no terms. No separate rowset/version/MoW issue was introduced.
- FE-BE variables: No new Thrift field is needed, but existing string-valued policy transport preserves a spelling that FE accepts and BE's exact factory lookup rejects.
- Performance: No new asymptotic hot-path or material memory-accounting issue was substantiated; repeated dictionary initialization is only a completed
call_oncecheck. - Other issues: After two full rounds, including separate lifecycle/phrase and dictionary/policy risk passes, all reviewers returned
NO_NEW_VALUABLE_FINDINGS. One char-filter candidate was disproven by concrete overrides and is intentionally not submitted.
Please address the eight inline findings before merge.
Contributor
TPC-H: Total hot run time: 16654 ms |
Contributor
TPC-DS: Total hot run time: 81543 ms |
Contributor
ClickBench: Total hot run time: 14.57 s |
airborne12
added a commit
to airborne12/apache-doris
that referenced
this pull request
Sep 13, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#67918 Problem Summary: The initial IK custom-analyzer integration did not fully preserve tokenizer reset semantics, document-relative UTF-8 offsets, case-insensitive built-in resolution, or upgrade compatibility when an existing named policy collides with a new built-in name. Its regression coverage also lacked generated golden output. Preserve the pending-reader reset contract, advance refill offsets by consumed bytes, rebase pinyin candidates to source-token byte spans, canonicalize built-in names while keeping named policies authoritative, accept IK tokenizer policy types in FE validation, and add focused unit and regression coverage. ### Release note None ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [x] Manual test - Behavior changed: - [x] No. This corrects edge cases in the feature introduced by the preceding commit. - [ ] Yes. - Does this need documentation? - [x] No. - [ ] Yes. Validation: - `./build.sh --be -j48` (ASAN) - `./build.sh --fe` - `./run-be-ut.sh --run --filter='IKTokenizerTest.*' -j48` (21 tests passed) - `./run-be-ut.sh --run --filter='AnalysisFactoryMgrTest.*:IKTokenizerTest.*:PinyinFilterTest.*:IndexPolicyMgrTest.*' -j48` (80 passed; one pre-existing order-dependent dictionary-initialization test failed, then passed in the isolated IK run) - `./run-fe-ut.sh --run --filter=org.apache.doris.analysis.invertedindex.AnalyzerIdentityBuilderTest` (6 tests passed) - `./run-fe-ut.sh --run --filter=org.apache.doris.indexpolicy.PolicyValidatorTests` (20 tests passed) - `./run-regression-test.sh --run -s test_ik_custom_analyzer -d inverted_index_p0/analyzer` (generated golden and clean comparison passed) - clang-tidy on all changed C++ lines with the ASAN compilation database; one test translation unit is blocked by the pre-existing unmatched `NOLINTEND` in `be/src/core/types.h` - C++ format, Checkstyle, build-hygiene, and glibc compatibility checks The Chinese literals in the tests and regression golden are tokenizer input and expected token data, not code descriptions.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
TPC-H: Total hot run time: 16855 ms |
Contributor
TPC-DS: Total hot run time: 82284 ms |
Contributor
ClickBench: Total hot run time: 14.94 s |
Contributor
There was a problem hiding this comment.
Request changes — static review completed and converged at exact head 33dd82008708152608064761581e49b9d1475744 against base 2573820600eb178150a35710a94fd58b478df96d.
Five distinct P1 findings are attached inline: aggregate BE test-order dependence, locale-dependent IK name normalization, loss of source-byte boundaries after fullwidth normalization, non-canonical built-in filter identities, and unequal identities for direct versus type-only named IK tokenizers.
Critical checkpoint conclusions:
- Goal and proof: the patch registers both IK modes and adds FE, BE, and regression coverage, but the goal is not safely complete while the combined BE test binary is order-dependent and the offset/identity cases above remain uncovered.
- Scope: the production change is focused on analyzer registration, reset/offset propagation, policy validation, and identity. All 19 changed files and their upstream/downstream paths were reviewed; no additional user focus was provided.
- Concurrency: factory registration and dictionary initialization use process-wide
call_once; FE policy maps remain lock-protected. No runtime race, lock-order, or deadlock issue was found. The singleton lifecycle does create the accepted cross-test failure. - Lifecycle: fresh/reusable custom streams, legacy raw-reader streams, scalar/ARRAY indexing, reader ownership, and repeated reset were traced; the current reset fix preserves ownership and tokenizes once per reader. Global dictionary test state is not isolated.
- Configuration: no new dynamic or persisted configuration key is introduced; both factories consume the existing dictionary path and keep smart/max-word mode per stream. No runtime configuration propagation gap was found.
- Compatibility: existing named-policy precedence is aligned across FE replay/identity and BE resolution, and unsupported mixed-version direct use fails explicitly rather than silently reinterpreting metadata. Locale normalization and semantic identity consistency still need the inline fixes.
- Parallel paths and conditions: tokenizer, token-filter, and char-filter resolution plus both CREATE TABLE and CREATE INDEX identity consumers were checked. The filter and named-wrapper branches are the missing parallel cases reported inline.
- Tests and results: regression ordering/goldens are structurally deterministic, but negative Unicode/locale/identity coverage is missing. Per review-runner contract I ran no builds or tests. Author-reported validation includes a combined relevant BE run failure followed by an isolated IK pass; live CI currently has formatter, Checkstyle, license, secrets, Cloud UT, and performance passing while BE UT, FE UT, and compile remain pending.
- Observability: existing exception and logging paths are adequate for this local analyzer feature; no new metric is warranted.
- Persistence and transactions: policy image/replay, name-map rebuilding, propagation, and drop/reference paths were traced. No EditLog payload, transaction protocol, or storage-format change is introduced.
- Data writes and crash behavior: index-writer scalar/ARRAY paths were checked; no new transactional atomicity or crash-leak issue was found beyond incorrect offsets and duplicate-index admission.
- FE/BE variables: no new thrift field or scattered session variable is introduced; existing policy properties carry the component names.
- Performance and memory: eager IK buffering is pre-existing; retaining two offsets per token adds bounded metadata and no new hot-loop or tracked-allocation regression was substantiated.
- Other correctness: error propagation and position increments are consistent. All prior threads were treated as hard duplicate fences, every candidate was resolved, and the third round returned
NO_NEW_VALUABLE_FINDINGSfrom all reviewers.
airborne12
added a commit
to airborne12/apache-doris
that referenced
this pull request
Sep 13, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#67918 Problem Summary: The IK analyzer integration could initialize global dictionaries from a factory-resolution test, depend on the JVM default locale, produce distinct identities for equivalent built-in components, and lose original byte boundaries when IK regularization changes UTF-8 width. Keep the factory-resolution test side-effect free, normalize names with Locale.ROOT, canonicalize all built-in component identities and type-only wrappers, and propagate source rune boundaries through token filters so pinyin offsets remain relative to the original document. ### Release note None ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [x] Manual test - Behavior changed: - [x] No. This corrects edge cases in the feature introduced by the preceding commits. - [ ] Yes. - Does this need documentation? - [x] No. - [ ] Yes. Validation: - ./build.sh --be -j8 (ASAN) - ./build.sh --fe -j8 - IKTokenizerTest and PinyinFilterTest (62 tests passed) - AnalysisFactoryMgrTest, IKTokenizerTest, PinyinFilterTest, and IndexPolicyMgrTest (82 tests passed) - AnalyzerIdentityBuilderTest and PolicyValidatorTests (29 tests passed) - clang-format 16 and Checkstyle passed - clang-tidy found no warnings on analyzable changed lines; one deletion-only test translation unit remains blocked by a pre-existing unmatched NOLINTEND in be/src/core/types.h The fullwidth literal in the test is tokenizer input data, not a code description.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review complete at head c5976f7cff0c8aae1f3dc174a377d1446b70c315 after two convergence rounds. Requesting changes for one distinct P1 issue described inline.
Critical checkpoint conclusions:
- Goal and test approach: The PR wires
ik_smartandik_max_wordthrough FE policy validation, BE factory construction, TOKENIZE/query, index writing, and regression coverage. The implementation goal is coherent, but semantic duplicate detection is incomplete across the legacy and custom IK entry points. - Scope and focus: Reviewed all 22 authoritative changed paths and their confirmed callers/consumers. The user supplied no additional focus, so the full PR remained in scope.
- Concurrency: FE policy-map locking, factory registration, and the process-wide IK dictionary initialization were traced; no distinct new runtime race or lock-order defect was found.
- Lifecycle and static initialization: Legacy/custom raw and shared readers, reusable scalar streams, ARRAY streams, reset/error recovery, dictionary lifetime, and filter span lifetime were checked. Residual test-order concerns are already covered by an existing thread and were not duplicated.
- Configuration: No new dynamic-config contract is introduced. Both IK modes use the established dictionary path; configuration normalization issues substantially covered by existing threads were fenced.
- Compatibility and rolling upgrade: No new persisted or wire schema is added. Policy replay/publication and existing-policy-first name collisions were checked; normal BE-first rollout is required for the new factories, with no separate compatibility defect found.
- Parallel and special-case paths: Checked smart, implicit-smart, and max-word modes; direct and named/type-only references; CREATE TABLE and ALTER/CREATE INDEX; scalar and ARRAY writes; TOKENIZE, MATCH, and query analysis. The cross-family duplicate gap affects both DDL paths.
- Conditionals, comments, and similar implementations: Compared legacy
IKAnalyzerconstruction withIKTokenizerFactory, reviewed normalization precedence and related tokenizer/filter factories, and traced source-offset/refill/truncation branches. No additional distinct issue survived the duplicate fences. - Tests: Changed BE unit, FE unit, and regression tests cover registration, validation, reset/reuse, refills, offsets, pinyin composition, both IK modes, TOKENIZE, and MATCH. Cross-family duplicate tests are missing and are requested with the inline finding. This reviewer ran no builds or tests because the review contract prohibited execution. The author reports ASAN builds, 82 combined BE tests, 29 FE tests, and the regression case; at submission time BE UT, FE UT, compile, performance, and review checks were still pending in CI.
- Outputs: The regression golden is ordered/deterministic for the asserted queries and follows cleanup-before-use conventions. No distinct output-contract issue was found.
- Observability and errors: Existing exceptions/status propagation and relevant logging remain in place; no new silent production failure or observability gap was found outside the identity bug.
- Persistence and transactions: Index-policy journal/replay/image reconstruction and FE-to-BE publication were traced. No transaction-state, metadata-format, or replay-order defect was found.
- Writes, atomicity, and crash behavior: Scalar/ARRAY index-writer consumption and reset ordering were reviewed; no new atomicity, partial-write, or crash-recovery issue was found.
- FE/BE variables and all consumers: No Thrift field is added. FE names and BE factories align for valid direct/named policies, but FE's analyzer identity does not unify the equivalent legacy IK representation, which is the blocking finding.
- Performance: Source-byte mapping is allocated only for width-changing normalization and the identity work remains DDL-side; no material new performance issue was substantiated.
- Other: Security review was not requested or implicated. Existing inline issues and replies were treated as hard duplicate fences and were not reposted.
Review status: complete static review; one new blocking finding; exact base/head verified before submission.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
airborne12
added a commit
to airborne12/apache-doris
that referenced
this pull request
Sep 13, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#67918 Problem Summary: Legacy IK parser indexes and equivalent custom analyzers used different semantic identities, allowing duplicate inverted indexes on one column. Canonicalize unfiltered legacy smart and max-word modes to the corresponding custom analyzer identities while preserving distinct identities for filtered legacy analyzers. ### Release note None ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - Behavior changed: - [x] Yes. Equivalent legacy and custom IK indexes are now rejected as duplicates. - Does this need documentation? - [x] No.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
TPC-H: Total hot run time: 16675 ms |
Contributor
TPC-DS: Total hot run time: 81320 ms |
Contributor
ClickBench: Total hot run time: 14.6 s |
Contributor
There was a problem hiding this comment.
Static review complete at exact head 0e66fd9522552abd9efa2bb685db6970859b19d1 against base 2573820600eb178150a35710a94fd58b478df96d after three bounded convergence rounds. Requesting changes for three P1 findings and one P2 finding attached inline.
Critical checkpoint conclusions
- Goal and proof: The PR exposes
ik_smartandik_max_wordto custom analyzers and covers the ordinary factory, reset, direct-offset, TOKENIZE, MATCH, CREATE, and ALTER paths. The happy path is coherent, but lazy error propagation, semantic identity, transforming-filter offsets, and unconditional resource costs keep the feature from being safely complete. - Scope and focus: The production change is generally focused on factory registration, tokenizer lifecycle/offsets, policy validation, and analyzer identity. All 22 authoritative changed files and their relevant callers/consumers were reviewed. No additional user focus was supplied, so the full PR remained in scope.
- Concurrency: Factory publication and dictionary initialization use the existing process-wide
call_oncemechanisms; FE policy-map access remains under the existing reentrant lock discipline. No new thread, race, heavy-under-lock operation, inconsistent lock order, or deadlock issue was found. - Lifecycle/static initialization: Raw and shared readers, eager and reusable streams, reset-before-consume, EOF behavior, and source-map lifetime were traced. Reset and ownership are sound on supported production paths. The dictionary singleton itself is stable, but its new lazy creation point exposes the uncaught error boundary reported inline; no cross-TU static-order hazard was found.
- Configuration: No new Doris runtime configuration key is added. Both modes use the established IK dictionary path and per-stream mode. No dynamic-configuration propagation gap was found.
- Compatibility: No wire or storage-format field changes. Direct/case-variant factories align across FE and BE, and supported BE-before-FE rolling order avoids an old-BE factory gap. Replayed pre-upgrade tokenizer-name collisions are nevertheless misused by the new synthetic legacy identity, as reported inline.
- Parallel paths: CREATE and ALTER share the faulty identity. TOKENIZE, row-fallback MATCH, and analyzed SEARCH all reach lazy tokenizer creation after their effective CLucene conversion boundary. Scalar/ARRAY writer and VARIANT metadata paths were separately traced; legal FE metadata does not route a custom analyzer into ARRAY ingestion, so that unsupported path is intentionally excluded from the finding.
- Special conditions: The conditional reset is safe in the traced caller sequences. The legacy-identity guard omits behavior-affecting state, and term-text-only source-map forwarding is not compositional through a supported transforming filter; both are concrete inline findings.
- Test coverage: Changed tests cover both modes, default legacy/custom duplicate detection in CREATE and ALTER, raw/shared reset contracts, synthetic IndexWriter ARRAY consumption, refill/direct full-width offsets, and deterministic TOKENIZE/MATCH results. Missing cases correspond to the four findings: first-use dictionary failure, legacy
lower_case=false, replay-plus-duplicate identity, transforming-filter source offsets, and resource behavior. The residual dictionary test-order concern is already covered by an existing thread and was not reposted. - Test results: The regression uses ordered/deterministic output, pre-test drops, hardcoded table names, and expected-error form. Per the review contract, I ran no build or test, so these conclusions are static. Author-reported ASAN builds, combined BE tests, FE tests, and regression execution were treated as author evidence, not independent verification.
- Observability: Existing dictionary logging is sufficient once failures are converted into Doris errors; no new metric is warranted. Letting a raw
CLuceneErrorescape is an error-contract defect, not an observability substitute. - Persistence and transactions: Policy image/replay/name-map rebuilding was traced. No EditLog payload, transaction state machine, failover, or metadata-format change is introduced; replayed-name state is relevant only to the identity finding.
- Data writes and crash behavior: Analyzer output affects inverted-index term generation, but no rowset/version/MoW, atomicity, partial-write, or crash-recovery defect was found. Direct offset/reset writer paths are sound; the supported transforming-filter chain still publishes incorrect source offsets.
- FE-BE variables: No new Thrift or session variable is added. Existing string-valued policy properties carry the tokenizer/filter names; ordinary resolution agrees, with the synthetic-identity exception reported inline.
- Performance and memory: The new source-map detector and enlarged eager token records affect every legacy/custom IK stream even when Pinyin is absent. This definite redundant scan and retained-memory growth is the P2 finding; no separate RTTI or memory-accounting finding was substantiated.
- Other issues: Existing rendered threads and raw comments were treated as hard duplicate fences. Every candidate was ledgered and adjudicated. The final round narrowed supporting evidence but did not add or remove a finding; the stable four-finding set is complete within the three-round bound. Security review was neither requested nor implicated.
airborne12
added a commit
to airborne12/apache-doris
that referenced
this pull request
Sep 13, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#67918 Problem Summary: Lazy IK dictionary failures could escape the custom analyzer exception boundary, legacy IK identities could collide with behaviorally different or shadowed custom tokenizers, and pinyin offsets lost source-byte provenance after word delimiter processing. Source offset maps were also built even when no downstream filter consumed them. Translate analyzer construction errors, keep legacy identity matching limited to equivalent built-in IK behavior, and make source-byte provenance opt-in and compositional through token filters. Preserve and rebase provenance for generated word delimiter parts and concatenations. ### Release note None ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - Behavior changed: - [x] Yes. IK custom analyzers now report Doris analyzer errors, keep distinct identities for non-equivalent legacy settings, and preserve exact source offsets through delimiter and pinyin filters without unconditional mapping work. - Does this need documentation? - [x] No.
Member
Author
|
run buildall |
### What problem does this PR solve? Related PR: apache#67918 Problem Summary: The analyzer identity regression used inline assertions for deterministic MATCH query results. Use ordered qt queries and runner-generated output so these cases follow the repository's regression conventions. Preserve the positive full-term matches and the negative keyword-fragment match. ### Release note None ### Check List (For Author) - Test: - run-regression-test.sh generated the new output (1 suite passed), then verified test_analyzer_identity_semantics and test_ik_custom_analyzer against this worktree's synchronized FE/BE ASAN output (2 suites passed). - The synchronized implementation also passed 137 related BE ASAN tests, 115 related FE tests, and the full native FE/BE ASAN build. - Behavior changed: No. This records the existing regression expectations. - Does this need documentation: No. This is a regression-only change.
airborne12
requested review from
924060929,
englefly,
gavinchou,
morrySnow and
starocean999
as code owners
September 18, 2026 09:56
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review converged after two complete normal/risk-focused rounds. I am requesting changes for three data-correctness issues: gapped WordDelimiter concatenations corrupt downstream Pinyin offsets; distinct IK configurations share a MATCH selection key; and named character-filter identities ignore transformations absorbed by the downstream IK tokenizer.
Critical checkpoints:
- Goal, proof, scope, and focus: the cross-cutting implementation is focused on IK custom-analyzer support, policy binding, semantic identity, MATCH dispatch, and source-offset propagation. The changed tests are broad, but their current oracles do not distinguish the three failures below. No additional user focus was specified.
- Data correctness and FE/BE propagation: the three inline comments trace concrete term/offset or reader-selection failures through the relevant FE, Thrift, and BE paths. Implicit/explicit MATCH, slow analysis, SNII, variant SEARCH, scoring, and classic writer paths were checked where applicable.
- Concurrency, lifecycle, and static state: FE policy maps remain lock-protected, BE maps remain protected by the existing shared mutex, replay/image/drop restore exact-name state, and token-stream reset paths clear their buffers. No distinct lock-order, stale-state, or initialization issue survived review.
- Compatibility, configuration, persistence, and transactions: no new wire/storage field or runtime configuration is introduced. Supported BE-first rollout, journal/image reconstruction, policy reconciliation, and transaction/data-write boundaries were checked; residual exact-name lifecycle concerns are already covered by existing threads.
- Error handling, ownership, nullability, observability, and resources: no new unhandled status/exception boundary, ownership/null path, or observability requirement was found. The remaining ICU dense-edit resource concern is already covered by an existing inline thread.
- Tests and validation: this review was static only, as required by the review prompt; no build or test was run. Author/CI-reported results are not independent validation. Add discriminating regression/unit oracles for each inline reproduction.
…ings ### What problem does this PR solve? Related PR: apache#67918 Problem Summary: Explicit built-in IK requests could select a smart or differently configured physical index after distinct IK identities were admitted on one column. Named A-to-a character replacement was not folded through IK's default lowercasing, and word delimiter concatenation discarded source gaps needed by downstream pinyin offsets. Encoded selection keys could also collide with arbitrary legal policy names. Match built-in IK indexes by effective configuration and carry the existing mode, lowercase and outer character-filter fields into BE selection keys. Separate encoded configurations from policy names without changing provider bindings, Thrift or stored index formats. Canonicalize named character filters in tokenizer context, and preserve optional per-rune source ends through concatenation and reset. Run the two new global-policy suites in the nonConcurrent group to avoid contributing to concurrent quota exhaustion. ### Release note Fix IK MATCH index selection and effective analyzer duplicate detection, and preserve pinyin source offsets through word delimiter concatenation. ### Check List (For Author) - Test - [x] Regression test Official runner: test_analyzer_identity_semantics and test_ik_custom_analyzer passed together; test_ik_analyzer passed separately. Expected output was generated by the runner and then checked in a normal run on a task-owned ASAN cluster. - [x] Unit Test Official ASAN BE runner: 178 tests across 12 related suites passed. After clang-tidy fixes, all 120 tests in the three affected suites passed again. Official FE runner: 118 tests in four classes passed. The three review defects and the encoded-name collision have discriminating native RED/GREEN evidence. - [ ] Manual test - [ ] No need to test or manual test. Explain why: - Additional validation Native build.sh --be --fe passed; BE was rebuilt after the final source change. Java Checkstyle and build hygiene passed. Clang-tidy checked all 12 newly changed C++ files and rechecked the three files subsequently corrected, with no checks disabled. The run remains qualified by pre-existing processCurrentToken size/complexity diagnostics and diagnostics in unchanged CLucene and Boost headers. A baseline-only unmatched NOLINTEND comment was removed through a temporary VFS overlay, not by changing repository code. A controlled baseline source comparison confirmed the function's complexity decreased from 75 to 69 and its length from 141 to 125 lines. These are not claimed as a clean tidy pass. - Behavior changed: - [ ] No. - [x] Yes. Explicit built-in IK uses its matching default configuration; distinct physical configurations retain separate selection keys, equivalent named character filters are rejected as duplicates, and concatenated tokens retain their original source spans. - Does this need documentation? - [x] No. This corrects existing analyzer behavior and test isolation. - [ ] Yes.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static exact-head review found one blocking rolling-upgrade compatibility issue, attached inline.
Checkpoint conclusions:
- Goal/tests: the IK tokenizer, offset, identity, and current-version reader-selection paths have broad focused coverage, but the old-FE/new-BE request boundary is untested.
- Focus/minimality: all 49 changed files are relevant to analyzer construction, policy/identity handling, offset provenance, physical selection, or their tests.
- Concurrency/lifecycle/persistence: FE and BE exact/normalized maps are lock-protected and rebuild/reconcile deterministically; no distinct current-version replay, drop, retry, or failover defect survived review.
- Configuration/observability: no new operator configuration is introduced; the blocking case is silent apart from the inverted-index downgrade counter and debug logging.
- Compatibility/parallel paths: legacy and custom analyzers, implicit and explicit MATCH, CLucene/SNII selection, and the documented BE-before-FE upgrade order were traced. Exact reader keys are not compatible with the base FE request shape.
- Conditions/data flow: mode, lowercase, outer filters, reset/reuse, UTF-8 clipping, deletion/expansion, and FE-to-BE analyzer fields were checked. Other technically valid concerns are already captured by live inline threads and were not reposted.
- Performance: the blocking case converts an indexed MATCH into per-row analysis on upgraded BEs until FE upgrade.
Validation was static-only as required by the review contract: I did not run builds or tests. The PR body reports successful author-side BE, FE, regression, formatting, and partial clang-tidy validation, but those claims were not independently executed here. Three normal reviews, four targeted risk reviews, and a three-way convergence round completed; all final-round reviewers returned NO_NEW_VALUABLE_FINDINGS beyond the single inline issue.
Contributor
TPC-H: Total hot run time: 27512 ms |
Contributor
TPC-DS: Total hot run time: 152795 ms |
Contributor
ClickBench: Total hot run time: 23.85 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Older FE versions lowercase analyzer names in MATCH requests and index properties, while replayed policy names may retain their original spelling. Exact reader selection can therefore miss an existing inverted index during a backend-first upgrade. Resolve the provider and canonical policy name under the same policy lock. Return a lowercase metadata alias only when it resolves to the same policy and does not select a builtin analyzer. Prefer the canonical reader key and use the verified alias only when that key is absent. Forward the binding to both physical reader selection and similarity-statistics collection. Cover exact and normalized metadata, legacy normalizers, case-colliding policies, canonical-key precedence, builtin-name collisions, and scoring. Initialize the real IK test dictionary explicitly so the selection fixture does not depend on another test's global dictionary configuration. Related PR: apache#67918 ### Release note Preserve custom-analyzer MATCH index bindings during backend-first upgrades. ### Check List (For Author) - Test: - Native ASAN BE build passed with the GLIBC 2.17 compatibility gate. - Official ASAN BE runner passed 354 tests in 10 related suites. - Three discriminating native RED/GREEN cycles cover legacy exact metadata, normalized index metadata, and the builtin-name alias collision. - Official regression runner passed all three relevant analyzer suites on the task-owned native ASAN cluster, without expected-output edits. - A full mixed-version cluster upgrade was not run. - Behavior changed: Yes. MATCH reuses only a compatible canonical or verified legacy index, and never substitutes another policy or a builtin analyzer. - Does this need documentation: No. No new syntax, stored format, wire field, or configuration is introduced.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
TPC-H: Total hot run time: 27547 ms |
Contributor
TPC-DS: Total hot run time: 152719 ms |
Contributor
There was a problem hiding this comment.
Static review of exact head 6618a7099cb4a347a1e26d492d9144b3b60210b5 found two distinct blocking issues: lazy custom-IK failures cross classic Status boundaries as the wrong exception type, and analyzer identity still misses a redundant pre-ICU replacement. Existing inline threads were treated as hard duplicate fences.
Critical checkpoint conclusions:
- Goal and tests: the PR largely implements custom IK tokenizers, source-offset propagation, exact policy binding, and configuration-aware reader selection, with broad FE/BE/regression coverage; the two missing negative cases below prevent completion.
- Scope and focus: the 57-file change is broad but coherent around analyzer construction, identity, offsets, selection, and compatibility; no unrelated production change was found.
- Concurrency: FE/BE policy maps are lock-protected and deterministic collision restoration was traced; process-global IK initialization uses
call_once. No new deadlock or lock-order issue survived review. - Lifecycle: dictionary first-use/failure/reload and reusable token-stream reset were traced. The accepted BE finding is the remaining lifecycle/error-boundary defect.
- Configuration: no new dynamically mutable server setting is introduced; existing dictionary-path behavior is used.
- Compatibility: no Thrift field or storage-format change was added. The supported BE-first rolling shape, exact-name aliases, classic/Nereids senders, MATCH, scoring, SEARCH, and variant paths were traced without another distinct issue.
- Parallel paths: classic and SNII paths were compared; the classic writer/query/scoring boundaries do not match SNII's
doris::Exceptionhandling, which is the first finding. - Conditions and identity: built-in/custom collision, mode, lowercase, outer-filter, and ordered-filter conditions were reviewed. The reverse char-filter walk loses a valid downstream absorption fact, which is the second finding.
- Test coverage and results: changed unit and regression tests cover reset/reuse, offsets, exact binding, selection, and upgrade aliases, but do not cover either accepted failure. Review was static-only; I did not build or run tests, and author/CI test claims are not independent validation.
- Observability: existing policy and dictionary logs are sufficient for the reviewed paths; no additional metric requirement was identified.
- Persistence, transactions, and writes: journal/image rebuild, FE-to-BE policy reconciliation, dependency checks, and writer call chains were traced. No new transaction or atomicity issue survived beyond the exception escape below.
- FE/BE variables: all existing
TMatchPredicatefields and sender paths were checked; no missing variable propagation was found. - Performance: opt-in provenance, compact ICU correction runs, and reader-key lookup were reviewed; no distinct unfenced performance defect remained.
- Other issues: the final changed-file sweep and two convergence rounds left no unresolved candidate; substantially similar offset and exact-validation concerns were not resubmitted.
| ts = filter->create(ts); | ||
| } | ||
| return std::make_shared<TokenStreamComponents>(tk, ts); | ||
| } catch (const CLuceneError& e) { |
Contributor
There was a problem hiding this comment.
[P1] Keep lazy IK failures inside the classic Status boundaries
This catch changes a lazy IKTokenizerFactory::create() failure from CLuceneError to doris::Exception, but the classic call sites still catch only CLuceneError. InvertedIndexColumnWriter::new_inverted_index_field() invokes reusableTokenStream() inside a Status method, and FullTextIndexReader::query() invokes get_analyse_result() inside another Status method; with a missing or corrupt IK dictionary, the converted exception now escapes both instead of returning their analyzer error. The added test calls CustomAnalyzer::tokenStream() directly, so it does not cover either boundary. Please preserve the original exception here or teach every classic invocation boundary to convert doris::Exception to a non-OK Status, and add a writer/query failure-path test.
| } | ||
| identities.addFirst(filter); | ||
| // An earlier replacement can change the input of a later filter. | ||
| lowercaseIk = false; |
Contributor
There was a problem hiding this comment.
[P1] Preserve absorption through a downstream case-folding filter
Clearing lowercaseIk after every nonempty downstream filter still gives different identities to equivalent pipelines. For example, let lower_a={type=char_replace,pattern=A,replacement=a} and fold={type=icu_normalizer} with its default nfkc_cf; {tokenizer=ik_smart,char_filter=fold} and {tokenizer=ik_smart,char_filter=lower_a,fold} emit the same text, positions, and source offsets because nfkc_cf already maps A to a. Walking backward resolves fold, clears the absorption context here, and then retains lower_a, so CREATE/ALTER can admit both indexes. Please propagate the known nfkc_cf case-folding context backward (or canonicalize the composed pipeline) and cover this intervening-filter case.
Contributor
ClickBench: Total hot run time: 24.13 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
3 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?
Custom inverted-index analyzers could not use IK smart or max-word tokenization with their configured character and token filters. This adds the
ik_smartandik_max_wordtokenizer factories and policy validation.Composed pipelines need source-byte provenance across normalization, splitting, concatenation, trimming, and reset/reuse. Opt-in offset tracking propagates through IK, keyword, standard, ICU normalization, word-delimiter, and pinyin processing. Separate rune end offsets preserve gaps removed by delimiter concatenation.
Review fixes preserve exact saved policy bindings across replay, lifecycle operations, MATCH serialization, provider dispatch, physical reader selection, and scoring-statistics collection. FE matches explicit built-in IK to its default max-word configuration. BE selection keys distinguish mode, lowercase configuration, and effective outer character filters without changing provider names, Thrift, or stored index formats. Encoded configurations cannot collide with arbitrary policy names.
For backend-first upgrades, BE now binds an old FE's normalized analyzer name to the canonical saved policy and accepts a lowercase metadata key only when it resolves to the same policy. Exact metadata remains preferred. The compatibility alias cannot select a differently bound policy or a built-in analyzer with different tokenization.
Named character-filter identities account for transformations absorbed by the resolved tokenizer while preserving meaningful ordered transformations. The two new global-policy suites run in
nonConcurrentinstead of contributing to concurrent P0 policy-quota exhaustion.Review fix commits:
4119d0bf2e3,2558085d88f,6618a7099cb. Current candidate:6618a7099cb.Release note
Add IK smart and max-word tokenizers to custom inverted-index analyzers. Correct composed-tokenizer source offsets, preserve custom-analyzer MATCH bindings during backend-first upgrades, select the matching physical IK configuration, and fix effective duplicate-index identities.
Check List (For Author)
test_ik_custom_analyzer,test_analyzer_identity_semantics, and existingtest_ik_analyzerpassed together against the final task-owned native ASAN cluster. No expected output changed.be/src/core/types.h; repository checks and source are unchanged.Component-level replay, serialization, dispatch, reader selection, scoring selection, and current-version end-to-end regression were validated. A full mixed-version cluster upgrade was not run. Existing IK internal lowercasing behavior is unchanged; the uppercase SQL probe is not treated as proof of a lowercase-disabled runtime fix.