[feat](inverted-index) Support configurable ngram size difference - #67917
Open
airborne12 wants to merge 6 commits into
Open
[feat](inverted-index) Support configurable ngram size difference#67917airborne12 wants to merge 6 commits into
airborne12 wants to merge 6 commits into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Custom ngram tokenizers hard-code the allowed difference between `max_gram` and `min_gram` to 1. This prevents valid wider ngram ranges while offering no explicit override.
This change adds the `max_ngram_diff` tokenizer property, keeps 1 as the backward-compatible default, validates non-negative values in FE and BE, and adds unit and regression coverage for a 1-to-8 tokenizer.
### Release note
Allow custom ngram tokenizers to configure the maximum difference between `max_gram` and `min_gram` with `max_ngram_diff`.
### Check List (For Author)
- Test
- [ ] Regression test (coverage added; execution is pending CI)
- [x] Unit Test
- [ ] Manual test
- Behavior changed:
- [ ] No.
- [x] Yes. Custom ngram tokenizers can opt into a wider gram-size range.
- Does this need documentation?
- [ ] No.
- [x] Yes. The new tokenizer property should be added to the custom analyzer documentation.
Validation:
- `./build.sh --be -j48` (ASAN)
- `./run-be-ut.sh --run --filter='NGramTokenizerTest.*' -j48` (14 tests passed)
- `./run-fe-ut.sh --run org.apache.doris.indexpolicy.PolicyValidatorTests` (20 tests passed)
- `build-support/run-clang-tidy.sh --base origin/master --build-dir be/build_ASAN`
- C++ format and build-hygiene checks
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Member
Author
|
/review |
Member
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review result: request changes. This static review converged after two rounds on commit f418b9582533510b8195e5e1b9077a6554f3e059. Two P2 issues remain: the newly configurable range has no practical token-fan-out bound, and the regression does not verify the complete deterministic result.
Critical checkpoint conclusions:
- Goal and proof: The FE validator, persisted policy map, FE-to-BE transport, and BE factory implement configurable
max_ngram_diff; the added cases exercise normal 1..8 use. The goal is not safely complete because extreme newly valid widths can generate denial-of-service-scale work, and the end-to-end regression proves only two members of the result. - Scope and clarity: The production diff is small and focused. The missing resource policy and incomplete regression oracle are the two material gaps.
- Concurrency: No new thread, shared mutable state, lock acquisition, lock-order dependency, or atomic protocol is introduced. Existing policy distribution and lazy analyzer construction retain their established synchronization.
- Lifecycle/static initialization: No new owning lifecycle, release path, circular reference, or cross-translation-unit static-initialization dependency is introduced. Policies persist the property and analyzers consume it lazily through the existing lifecycle.
- Configuration behavior: This is persisted per-policy metadata, not a dynamically mutable process setting. Omitting it preserves the old default of 1; FE and BE agree on default, zero/positive acceptance, and negative rejection. A practical resource ceiling is still required.
- Compatibility: Existing policies without the property keep the old behavior, and same-version FE/BE validation is aligned. Doris's supported upgrade flow upgrades all BEs before FEs, with the Master FE last, so no compatibility issue survives under the documented order: https://doris.apache.org/docs/4.x/admin-manual/cluster-management/upgrade/.
- Parallel paths:
TOKENIZE, classic/SNII indexing, and query analysis reconstruct the same custom-analyzer configuration and converge on the same ngram factory; no parallel path drops the property. - Conditional checks: The new nonnegative and difference checks are understandable and mirrored across FE/BE, but they validate only arithmetic relation, not downstream work. The accepted resource-bound comment covers that omission.
- Test coverage: FE tests cover the default limit, configured success, and a negative limit; the BE test proves configured construction/output for a four-character input; the regression covers DDL, propagation, and lazy BE resolution. Upper/resource boundaries and the full 1..8 output are not adequately covered.
- Test results: The BE expected vector is correct for
abcd, but cannot emit sizes 5..8. The deterministicabcdefghregression should useqt_*plus a generated.outresult instead of two substring assertions. - Observability: This change does not add a distributed operation or state transition that needs new metrics/logs; existing validation errors identify the invalid property. Resource prevention, rather than extra logging, is needed for the accepted fan-out issue.
- Persistence/transactions: The generic Gson/EditLog policy representation preserves the property, replay uses the same map, and no new transaction or failover protocol is introduced.
- Data writes: There is no storage-format or atomicity change. Index writes consume the shared tokenizer factory, so the resource issue also applies to indexing, but no separate correctness/transaction issue was found.
- FE-to-BE propagation:
PushIndexPolicyTaskcopies the complete properties map intoTIndexPolicy, and BE policy installation retains it; no scattered transport path was missed. - Performance and memory: Wide low-minimum ranges make token work grow with both input length and configured width; the first inline comment gives a concrete 1.25-billion-token case. No separate allocation, CPU, or redundant-work issue remains after deduplication.
- Other issues/user focus: No additional user-provided focus was present, and the final changed-file/duplicate-fence sweep found no further substantiated issue.
No builds or tests were run because the review contract prohibits them; all conclusions are static, and any author/CI test claims were not independently reproduced.
Contributor
TPC-H: Total hot run time: 16635 ms |
Contributor
TPC-DS: Total hot run time: 81086 ms |
### What problem does this PR solve?
Problem Summary:
The configured ngram range test covered only a short input, and the regression assertion sampled two tokens without detecting missing, duplicate, or reordered output. Compare the complete deterministic 36-token sequence for sizes 1 through 8 in both BE unit and regression coverage.
### Release note
None
### Check List (For Author)
- Test
- [x] Unit Test
- [ ] Regression test
- Behavior changed:
- [x] No.
- Does this need documentation?
- [x] No.
Contributor
ClickBench: Total hot run time: 14.67 s |
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review of e3f68c19ec66a9740e6435fc998879da10227a6d against 2573820600eb178150a35710a94fd58b478df96d is complete. I found two new P2 correctness/invariant issues and request changes. I treated the existing token-fan-out and deterministic-output discussions as hard duplicate fences.
Critical checkpoints
- Goal and proof: The default ceiling of
1and configured1..8path are wired from FE validation through policy persistence/Thrift into the BE factory, and the changed tests check the complete 36-token sequence. The feature is not yet correct because analyzer identity includes a validation-only property and FE/BE disagree on accepted numeric text. - Scope and design: The five-file implementation is otherwise focused. Runtime-effective analyzer identity needs canonicalization rather than hashing the complete tokenizer-policy map.
- Concurrency: No new threads or shared mutable state are introduced. Existing policy-manager locking, per-call TOKENIZE/query streams, writer-local stream reuse, and matcher
call_onceinitialization are unchanged; no lock-order or deadlock issue was found. - Lifecycle: Analyzer/factory ownership and static matcher lifetime are unchanged. No new circular ownership, cleanup, or cross-TU initialization issue was found.
- Configuration: This is an immutable tokenizer-policy property, not a dynamically mutable process configuration. The default is consistently
1in FE and BE; dynamic-reload concerns do not apply. - Compatibility: The supported BE-first rolling order is safe: old FE rejects the property, upgraded BE preserves the old default, and new FE enables it only after BE upgrade. No symbol or storage-format change is introduced. The Unicode parsing mismatch in the second inline comment is nevertheless a value-level FE/BE incompatibility.
- Parallel paths: TOKENIZE, index writers, and query analysis all resolve the same policy and BE tokenizer factory. The property reaches all of them through the generic map path; no separate consumer was missed.
- Conditions and errors: Missing, ASCII zero/negative, valid-boundary, and range behavior align across FE and BE. Unicode decimal text is accepted by FE and rejected later by BE. Error conversion is otherwise unchanged.
- Tests and expected results: BE tests cover the configured range and all 36 ordered tokens; FE tests cover configured/default/negative validation; the regression case exercises DDL-to-BE propagation and full semantic output. Missing analyzer-identity and Unicode-parity cases correspond to the inline findings. The result-format concern is already covered by discussion
r4000105184and is not duplicated. - Observability: No new distributed or asynchronous operation is added, and existing analyzer-construction errors are surfaced; no new metric or logging requirement was identified.
- Persistence, failover, and transactions: Edit-log/image serialization and Thrift push preserve the generic property map. Policies are immutable create/drop objects with fresh IDs, so ID-based reconciliation does not retain an older value for the same policy. No transaction, atomicity, delete-bitmap, or storage-version path changes.
- Data writes: The new range affects token generation for inverted-index writes but does not change write transactionality or storage format. Writer/query analyzer parity was traced through the shared factory.
- FE/BE propagation: All generic property-map paths carry
max_ngram_diff; that unchanged-string propagation is exactly why the second finding fails late rather than at DDL validation. - Performance: The validation-only identity dimension can allow redundant indexes and their write/storage cost. The unbounded fan-out/resource concern is already raised in discussion
r4000105182and is not duplicated here. - Other: The user-visible property still needs documentation, as the PR description notes; no documentation file is included in this diff.
No additional user-provided focus was supplied, so I reviewed the full PR scope. Per the review contract I did not run builds or tests; this is a static review. The author's reported BE test and current CI formatting/checkstyle results are supporting evidence only, while BE UT, FE UT, and compile checks were still pending at submission time.
Contributor
TPC-H: Total hot run time: 16727 ms |
Contributor
TPC-DS: Total hot run time: 81183 ms |
Contributor
ClickBench: Total hot run time: 14.6 s |
Issue Number: close apache#67916 Related PR: apache#67917 Problem Summary: The max_ngram_diff creation limit incorrectly changed analyzer identity, allowing equivalent custom analyzers to bypass duplicate-index detection. FE also accepted non-ASCII digits that the BE integer parser rejects. Exclude max_ngram_diff from ngram tokenizer identity and require its value to use ASCII integer syntax. Release note: None Validation: - ./run-fe-ut.sh --run org.apache.doris.analysis.invertedindex.AnalyzerIdentityBuilderTest,org.apache.doris.indexpolicy.PolicyValidatorTests (27 tests passed) - FE Checkstyle passed as part of the targeted test run Behavior changed: Equivalent ngram analyzers now share an identity regardless of max_ngram_diff, and FE rejects non-ASCII max_ngram_diff values. Documentation impact: None
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
TPC-H: Total hot run time: 16848 ms |
Contributor
TPC-DS: Total hot run time: 82222 ms |
Contributor
There was a problem hiding this comment.
Review result: request changes; no new inline findings.
This static review converged after two rounds on exact head 89137eab9b7acc1b55534a6ea5dec8fc95304980 against base 2573820600eb178150a35710a94fd58b478df96d. The current head resolves the analyzer-identity and new-property Unicode-parsing findings, and the final seven-file sweep found no further distinct PR-introduced issue. Existing discussions r4000105182 and r4000105184 remain the authoritative blocking feedback, so I did not duplicate either inline comment.
Critical checkpoint conclusions:
- Goal and proof: The FE validator, persisted policy map, FE-to-BE delivery, and BE factory consistently implement configurable
max_ngram_diff, with the old default of1. The implementation correctly treats the ceiling as validation-only for analyzer identity. Safe completion is still blocked by the already-raised resource-bound concern. - Scope and design: The seven changed files are focused on validation, factory construction, identity, and tests. Excluding only
max_ngram_difffrom ngram-tokenizer identity preserves every runtime-effective tokenizer property. - Concurrency and lifecycle: No thread, lock-order, shared-mutable-state, ownership, cleanup, static-initialization, or reusable-stream lifecycle change is introduced. Fresh TOKENIZE/query streams and reusable writer streams construct the same factory, and tokenizer reset behavior is unchanged.
- Configuration and compatibility: This is immutable per-policy metadata rather than a dynamically reloadable process setting. Missing values preserve the default; FE and BE align on the configured comparison and negative rejection, and FE now rejects non-ASCII text for the new field before persistence. Existing policies retain old behavior. The mechanically risky new-FE/old-BE topology is outside Doris's documented BE-first upgrade order: https://doris.apache.org/docs/4.x/admin-manual/cluster-management/upgrade/.
- Propagation and parallel paths: The generic property map is preserved through EditLog/image replay, report-driven
PushIndexPolicyTaskdelivery, and BE policy storage. TOKENIZE, classic/SNII index writers, and query analyzers all converge onNGramTokenizerFactory; no consumer drops or reinterprets the property. - Conditions and errors: Default, configured, negative, non-ASCII, and range checks are understandable and aligned for the new field. The Round 1 Unicode min/max and leading-zero identity candidates are concrete older gaps, but authoritative base/head comparison confirms this PR neither introduces nor worsens them.
- Tests and result correctness: The BE unit case checks the full ordered 36-token sequence for sizes 1..8. FE cases cover default rejection, configured acceptance, negative/non-ASCII input, and identity behavior; the regression covers asynchronous policy arrival plus lazy BE construction through TOKENIZE. Its deterministic result still uses
assertEqualsinstead of the repository-requiredqt_*plus generated.out, which remains covered byr4000105184. No separate writer/search-only failure mechanism was found. - Persistence, failover, transactions, and writes: The existing generic policy serialization/replay format already carries the new property, with no schema or storage-format change. No transaction, atomicity, delete-bitmap, or failover protocol changes. Index writes share the tokenizer factory; no distinct write-correctness issue was found.
- Performance and memory: Very wide low-minimum ranges can still cause quadratic token work, TOKENIZE materialization, and synchronous postings work. That exact issue remains hard-fenced by
r4000105182; no distinct new overflow, cancellation, or allocation finding survived deduplication. - Observability: Existing validation/analyzer-construction errors identify invalid policy values, and this patch adds no new distributed state transition requiring metrics or logging. Prevention, rather than added observability, is the relevant response to the existing resource concern.
- Other and user focus: No additional user-provided focus was supplied. The PR describes user-visible syntax and still marks documentation as needed, but no documentation file is included in this diff.
No builds or tests were run because the review contract prohibits them. The author reports 14 BE tokenizer tests and 27 focused FE tests passing; those claims were not independently reproduced. At submission time, Checkstyle, Clang Formatter, license, title, and secret checks pass, while BE UT, FE UT, compile, and performance checks remain pending.
Review completion: complete after two rounds. Every candidate is dismissed with concrete evidence or fenced by existing GitHub feedback; there are no accepted new inline comments. The request-changes opinion remains solely because r4000105182 and r4000105184 are unresolved.
Contributor
ClickBench: Total hot run time: 14.72 s |
### What problem does this PR solve? Issue Number: close apache#67916 Related PR: apache#67917 Problem Summary: An arbitrarily large max_ngram_diff could multiply token output without a hard fan-out bound, and the deterministic regression assertion was not stored as a runner-generated golden. Cap max_ngram_diff at 255 consistently in FE and BE, cover the accepted and rejected boundaries, and replace the manual token-list assertion with a named golden query generated by the regression runner. ### Release note The max_ngram_diff tokenizer setting accepts values from 0 through 255. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [x] Manual test - Behavior changed: - [ ] No. - [x] Yes. max_ngram_diff values above 255 are rejected to bound per-position token fan-out. - Does this need documentation? - [ ] No. - [x] Yes. Document the supported max_ngram_diff range. Validation: - ./build.sh --be -j8 (ASAN, Java extensions, build hygiene, and glibc compatibility) - ./build.sh --fe -j8 - NGramTokenizerTest: 16/16 passed - AnalyzerIdentityBuilderTest and PolicyValidatorTests: 29/29 passed - test_ngram_max_diff_custom_analyzer: runner-generated golden and clean comparison passed on an isolated local FE/BE - clang-format 16, clang-tidy, and Checkstyle passed
Member
Author
|
/review |
Member
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
Review result: request changes. This static review converged after three bounded rounds on exact head c52bb7f1e59cd37992f4f48ba08cf90e926c1232 against base 2573820600eb178150a35710a94fd58b478df96d. Two distinct P2 issues remain: newly admitted small-width policies can reach unsafe absolute-size allocation, and analyzer identity collapses a usable replacement with replayed legacy-invalid state. Existing discussions about width-driven fan-out, deterministic output, valid-policy identity, and Unicode parsing were treated as hard duplicate fences.
Critical checkpoints
- Goal and proof: FE and BE both implement
max_ngram_diff, preserve the old default of1, and accept the intended1..8configuration with an explicit limit of7. The complete 36-token unit and regression expectations prove the normal emitted sequence statically. The goal is not safely complete because M-1 admits absolute gram sizes that fail on first use and M-2 blocks repair of legacy persisted state. - Scope and design: The nine-file patch is focused on validation, identity, and tests. The cross-language ceiling is clear, but absolute-size safety and legacy-state-aware identity canonicalization are missing.
- Concurrency: No new thread, lock acquisition, lock order, shared mutable setting, or atomic protocol is introduced. Existing policy-manager locking and function-local
once_flagmatcher initialization are unchanged. - Lifecycle and static initialization: Policy installation remains lazy; analyzer/factory ownership and tokenizer stream lifetimes are unchanged. No circular ownership, release-path, or cross-translation-unit initialization issue was found. That lazy lifecycle is relevant to M-1 because DDL and synchronization succeed before tokenizer allocation.
- Configuration behavior: This is immutable persisted per-policy metadata, not a dynamically reloadable process setting. FE and current BE align on absence/default
1, ASCII syntax for the new field, and inclusive0..255bounds. The duplicated limit has corresponding boundary coverage. - Compatibility: Existing valid policies without the property preserve width-one behavior. The documented supported rolling procedure upgrades all BEs before FEs, so the unsupported new-FE/old-BE topology is not reported: https://doris.apache.org/docs/4.x/admin-manual/cluster-management/upgrade/. Supported replay can still retain base-FE-created wide policies without the property; M-2 covers that separate persisted-state defect.
- Parallel paths: TOKENIZE, classic inverted-index writing, SNII writing, and query analysis all resolve the same custom analyzer and
NGramTokenizerFactory; no path drops or reinterprets the new property. - Conditions and error handling: Normal default, configured, negative, non-ASCII, 255, 256, and ordered-range checks are understandable and aligned. No absolute gram bound, checked
size_tcalculation, or allocation-safe boundary exists beforeNGramTokenizer::init; replay also does not mark legacy-invalid n-gram policies. - Tests and expected results: The BE unit vector and generated regression golden contain the complete deterministic 36-token result. FE/BE tests cover configured success, default rejection, negative/non-ASCII input, and 255/256. Missing oversized-allocation and replay-plus-replacement cases map directly to M-1 and M-2. The identity test's no-limit
1..8control is not runtime-valid and masks M-2. No separate writer test is required to expose another path because all writers share the factory. - Observability: Existing validation and analyzer-construction errors are adequate for ordinary failures, and this patch adds no new distributed state transition requiring metrics. Replayed invalid policy state lacks a diagnostic/marker, as covered by M-2.
- Persistence and transactions: The generic Gson/EditLog/image and Thrift property maps preserve
max_ngram_diff; no new transaction or failover protocol is added. Replay without current validation is the compatibility exception in M-2. - Data writes and storage: There is no storage-format, data-version, atomicity, delete-bitmap, or merge-on-write change. Index writers consume the same lazy tokenizer, so M-1 applies to writes without creating a separate data-correctness finding.
- FE-to-BE propagation:
PushIndexPolicyTask, report reconciliation, and BE policy storage carry the complete property map. New FE/current BE semantics agree; no capability gate exists, but supported BE-first order handles mixed versions. - Performance and memory: M-1 is the remaining resource defect. Its exact width-two cases are newly reachable even though the underlying width-zero/one absolute-size weakness predates this PR. The earlier width-driven token fan-out issue is already covered by
r4000105182and is not duplicated. - Other issues and user focus: No additional user-provided focus was supplied. The user-visible property has no documentation change in this diff, but no separate correctness issue was substantiated.
Per the review contract, I did not run builds or tests and did not modify repository source. These conclusions are static; author and CI test claims were not independently reproduced.
### What problem does this PR solve?
Problem Summary:
Large absolute ngram sizes could allocate an excessive tokenizer buffer even when max_ngram_diff was small. Persisted ngram policies that became invalid under current validation could also collide with a valid replacement analyzer identity.
Cap custom ngram tokenizer sizes at 1024 in FE and BE, use overflow-safe buffer sizing, reject references to invalid replayed tokenizer policies, and give those policies stable policy-specific identities.
### Release note
None
### Check List (For Author)
- Test
- [x] Regression test
- [x] Unit Test
- Behavior changed:
- [x] Yes. Unsafe or currently invalid ngram tokenizer policies are rejected before analyzer construction.
- Does this need documentation?
- [x] No.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
TPC-H: Total hot run time: 16883 ms |
Contributor
TPC-DS: Total hot run time: 82016 ms |
Contributor
ClickBench: Total hot run time: 14.7 s |
Contributor
There was a problem hiding this comment.
Request changes: one P1 backward-compatibility defect remains.
The configurable max_ngram_diff path is otherwise focused and internally consistent, but the new absolute gram-size ceiling is applied to metadata that was valid under the previous release. That makes the feature unsafe for a supported rolling upgrade until legacy policies are preserved or migrated.
Checkpoint conclusions:
- Goal and proof: FE and BE implement the default limit of 1 and configured range through 255, and the unit/regression changes cover validation boundaries and the complete deterministic 36-token result. The goal is not fully achieved because the accepted size cap breaks existing policies during upgrade.
- Scope and clarity: the implementation is localized to tokenizer validation/allocation, policy validity/identity, and focused tests. All 13 changed files and their producer/consumer paths were reviewed.
- Concurrency: no new threads or shared mutable state are introduced. Existing
IndexPolicyMgrmap access remains under its read/write lock, with no new heavy operation under lock, lock-order change, or deadlock path. - Lifecycle and static initialization: tokenizer factories remain immutable after initialization and no cross-TU static dependency or ownership cycle was added. Image/edit-log replay recomputes validity with the current binary; that lifecycle choice is the accepted compatibility defect.
- Configuration:
max_ngram_diffis persisted policy metadata rather than a dynamic process configuration, so live config propagation is not applicable. - Compatibility and protocol: the existing generic FE-to-BE property map needs no schema change, and the documented BE-first upgrade order prevents new-FE/old-BE wider-range DDL. It does not protect already persisted fixed-size policies above 1024: upgraded BEs reject lazy reconstruction before FE upgrade, and upgraded FEs then reject analyzer references.
- Parallel paths and conditions: CREATE ANALYZER, CREATE TABLE, ALTER ADD INDEX, direct TOKENIZE, classic/SNII writers, query analyzer creation, policy report/push, and identity fallback were traced. Head FE/BE validation agrees; the unchanged unbounded
edge_ngrampath is a pre-existing hardening gap, not a distinct regression. - Error handling, memory, and nullability: invalid head policies fail with explicit validation/construction errors; no unchecked
Status, new ownership problem, column-nullability path, or unsafe bounded arithmetic was found. With the new cap, the n-gram buffer is bounded to 5120 code-point slots. - Tests and results: the added boundary, invalid-input, identity/replay, and end-to-end golden coverage is internally consistent. The missing case is an image/edit-log upgrade test for an existing valid fixed-size policy above 1024 and its dependent index. No builds or tests were run in this static-only review; test claims in the PR and thread replies are author-provided.
- Observability: replay warnings contain policy name, ID, and type; no additional metric is warranted for this DDL path. Logging does not preserve availability for the incompatible metadata.
- Persistence, transactions, and data writes: policy create/drop journaling, replay, image rebuild, and generic BE synchronization were reviewed. No storage-format, transaction, visible-version, delete-bitmap, atomicity, or data-write logic changes are involved; failover is otherwise symmetric.
- FE/BE variables and performance: the property reaches BE through every discovered policy path. The 255 difference cap bounds per-position fan-out and the 1024 cap bounds allocation; earlier fan-out/allocation/output concerns are already covered by existing threads. No additional distinct performance defect survived review.
- Documentation and user focus: the new policy property and limits should be documented after compatibility is resolved. No additional user-provided focus was supplied.
Review status: complete static review at exact head 8de9abb95d0c17da0588b2196ccf8ea135477801. Two full review rounds converged with every candidate accepted, duplicate-fenced, or dismissed with evidence.
### What problem does this PR solve?
Problem Summary:
The absolute ngram size limit was applied during replay and BE reconstruction, so policies accepted before the limit existed could become unusable during a supported rolling upgrade.
Persist an explicit compatibility marker on newly created ngram policies and enforce the absolute size limit only for marked policies. Preserve the former validation and construction behavior for marker-less legacy metadata. Cover serialized replay, dependent analyzer validation, marker persistence, and BE factory construction.
### Release note
None
### Check List (For Author)
- Test
- [x] Unit Test
- Behavior changed:
- [x] Yes. Legacy ngram policies remain usable after upgrade while newly created policies retain the absolute size limit.
- Does this need documentation?
- [x] No.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated static review conclusion: no new distinct blocking issue at head 035a049 against base 2573820. Two bounded review rounds converged, all existing inline discussions were used as hard duplicate fences, and no new inline comment remains.
Critical checkpoint conclusions:
- Goal and proof: The patch implements configurable max_ngram_diff with the prior default of 1, a 0..255 fan-out limit, a 1024 absolute-size bound for newly created policies, and marker-less compatibility for policies persisted by an older FE. The BE unit, FE unit, replay/identity, and regression changes statically cover the intended 1..8 sequence, validation boundaries, marker persistence, legacy 2048 reconstruction, and deterministic output.
- Scope and clarity: All 13 changed files are focused on tokenizer validation/allocation, policy compatibility and identity, and their tests. No unrelated production behavior was added.
- Concurrency: No new thread, atomic, or lock order is introduced. IndexPolicyMgr continues to protect its policy map with the existing read/write lock; the added property parsing and map insertion under creation are bounded, lightweight operations, and no deadlock path was found.
- Lifecycle and static initialization: No new ownership cycle, cleanup protocol, reusable-stream lifecycle change, or cross-translation-unit static dependency is introduced. The relevant lifecycle is persisted policy creation, image/EditLog replay, FE report/push, BE storage, and lazy analyzer reconstruction; the compatibility marker survives that complete path.
- Configuration behavior: max_ngram_diff is immutable per-policy metadata, not a dynamically reloadable process configuration, so dynamic-change handling is not applicable.
- Compatibility: The generic property map requires no Thrift or storage-schema change. New FE policies always carry the marker and are capped; replay does not synthesize it, so policies created before upgrade retain the former fixed-width behavior. This is consistent through the supported BE-first rolling-upgrade order.
- Parallel paths: TOKENIZE, query analysis, and classic/SNII index writers converge on the same ngram factory and property map. The separate edge_ngram validator/factory remains uncapped, but exact base/head arithmetic shows the same multi-GiB request was already reachable before this PR; it is a pre-existing parallel instance of the absolute-size mechanism already fenced by discussion 4000732693, not a distinct PR finding.
- Conditions and errors: Default, explicit, ASCII syntax, positivity/order, 0, 255, 256, marker-presence, and 1024 boundary conditions align between FE and BE. Existing analyzer-validation and construction errors are propagated; no unchecked Status, nullability path, or speculative recovery branch was added.
- Test coverage: The patch includes focused BE and FE unit coverage plus an end-to-end regression path. Negative cases cover malformed and out-of-range input, marked oversized metadata, invalid replayed state, and valid replacement identity. The complete 36-token sequence supplies a deterministic oracle for the newly enabled range.
- Test-result correctness: The checked-in unit expectations and generated regression result are internally consistent with the tokenizer loop and ordered sequence. A trailing blank line in the generated result is the only git diff-check warning and does not change the oracle.
- Observability: This patch adds no new distributed state transition or long-running background operation. Existing validation, replay, and analyzer-construction diagnostics are sufficient; no additional metric or INFO log is warranted.
- Persistence, failover, and transactions: The policy map is written and replayed through the existing Gson/EditLog/image mechanisms, and replay preserves marker presence exactly. Master failover reconstructs the same state. No transaction protocol is changed.
- Data writes: There is no storage-format, visible-version, delete-bitmap, atomicity, or data-mutation protocol change. Index writers use the same validated factory; no separate write-correctness defect was found.
- FE-to-BE propagation: ReportHandler and PushIndexPolicyTask carry the complete generic property map, BE stores it unchanged, and analyzer reconstruction observes the same marker and numeric values on every discovered path.
- Performance and memory: The 255 difference bound limits per-position fan-out, marked policies cap gram size at 1024, and the allocation arithmetic is widened to size_t. Legacy marker-less behavior is intentionally preserved for compatibility. No new distinct CPU, allocation, overflow, or cancellation issue survived the PR-scope and duplicate fences.
- Other issues and user focus: No additional user-provided review focus was supplied. The user-visible property still needs its planned documentation, but no separate code-correctness issue was found.
No builds or tests were run because the review contract prohibits them. The author reports focused BE/FE tests and full ASAN builds; those claims were not independently reproduced. At the pre-submission check, CheckStyle, Clang Formatter, license, title, secret, and Cloud UT checks pass, while BE UT, FE UT, compile, performance, and the automated code-review checks remain pending.
Review status: complete static review after two rounds. No new inline findings.
Contributor
TPC-H: Total hot run time: 16734 ms |
Contributor
TPC-DS: Total hot run time: 82100 ms |
Contributor
ClickBench: Total hot run time: 14.66 s |
Member
Author
|
run buildall |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 16789 ms |
Contributor
TPC-DS: Total hot run time: 82619 ms |
Contributor
ClickBench: Total hot run time: 14.84 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && 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
|
csun5285
approved these changes
Sep 14, 2026
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
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number:
Related PR: None
Problem Summary:
Custom ngram tokenizers hard-code the allowed difference between max_gram and min_gram to 1. This prevents valid wider ngram ranges while offering no explicit override.
This PR adds max_ngram_diff, keeps 1 as the backward-compatible default, requires ASCII integer syntax, and validates the same 0 through 255 range in FE and BE. The upper bound limits per-position token fan-out. Newly created policies also cap absolute custom ngram sizes at 1024, while an explicit persisted compatibility marker preserves marker-less policies accepted before that cap during rolling upgrades. Invalid replayed policies cannot block valid replacements. Because max_ngram_diff controls policy admission but does not affect emitted tokens, valid policies exclude it from analyzer identity so equivalent analyzers cannot bypass duplicate-index detection.
Release note
Allow custom ngram tokenizers to configure the maximum difference between max_gram and min_gram with max_ngram_diff values from 0 through 255.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Validation: