Uh oh!
There was an error while loading. Please reload this page.
branch-4.1: [fix](fe) block inverted index V1 creation in FE #64522 - #67342
Conversation
hello-stephen
commented
Aug 31, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
airborne12
commented
Aug 31, 2026
run buildall |
airborne12
commented
Aug 31, 2026
run compile |
1 similar comment
airborne12
commented
Aug 31, 2026
run compile |
airborne12
commented
Aug 31, 2026
/review |
airborne12
commented
Aug 31, 2026
COMPILE has failed 4 times (builds 1034854, 1034897, 1035119, 1035179) across 3 different agents (172.16.0.44, .15, .12), all with the identical error: building thirdparty This PR only touches FE Java + regression groovy files, so it cannot affect the thirdparty build. Sibling branch-4.1 PR #67329 compiled green at 17:43 today, between two of our failures, so the branch tip and toolchain are fine — it looks like the prebuilt thirdparty cache misses for this PR's compile jobs and the source-build fallback is broken. Triggering one more full run; if it fails at the same point again, this needs CI-side attention. |
airborne12
commented
Aug 31, 2026
run buildall |
There was a problem hiding this comment.
Automated review status: request changes.
Critical checkpoints:
- Admission and compatibility: startup/runtime config changes and ordinary new-table creation are covered, and replay/restore/read/load/compaction plus BUILD of committed V1 indexes remain outside the new rejection as intended. However, pre-format-property V1 tables surface as DEFAULT and still bypass new INVERTED/ANN admission.
- Regression migration: one active non-cloud P2 success suite still issues explicit V1 DDL, and two format-independent fault-injection obligations were lost or made unreachable.
- Test oracles: the changed golden-output sequences align with the retained calls; stale trailing sibling blocks were investigated and dismissed because the harness ignores them without changing current assertions.
- User focus: no additional review focus was supplied.
- Convergence: two complete rounds finished, every candidate was accepted, deduplicated, or dismissed with evidence, and all Round 2 reviewers reported NO_NEW_VALUABLE_FINDINGS.
| } | ||
| if (indexDef.getIndexType() == IndexType.INVERTED | ||
| && olapTable.getInvertedIndexFileStorageFormat() == TInvertedIndexFileStorageFormat.V1) { |
There was a problem hiding this comment.
[P1] Treat the legacy DEFAULT sentinel as V1
Tables created before this property existed deserialize its missing FE value as DEFAULT, while their physical tablet schema defaults to V1. This exact-enum check lets ADD INVERTED INDEX through, and ADD ANN also passes its own exact-V1 check. With the default light-index-change path, the format-free task makes BE copy that legacy schema and build against V1 (including ANN, which is unsupported there); with light change disabled, DEFAULT is instead mapped to V3 and silently upgrades the table. Please resolve/reject the effective legacy V1 value before both index types while leaving replay/restore/BUILD of committed indexes unchanged.
There was a problem hiding this comment.
Thanks — I checked this against master and the same code is there: SchemaChangeHandler on master carries the identical exact-enum guard (getInvertedIndexFileStorageFormat() == TInvertedIndexFileStorageFormat.V1), and this PR ports it verbatim (the only local adaptation in that hunk was keeping 4.1's indexDef.getColumns(), since getColumnNames() is master-only drift).
So the legacy DEFAULT sentinel behaviour you describe is not introduced by this backport — it exists identically on master after #64522. Changing it only on branch-4.1 would make the two branches diverge, which is exactly what a backport should avoid. Keeping this PR faithful to upstream; the DEFAULT-sentinel handling is worth a separate fix on master that can then be picked to 4.1 in the same shape.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
hello-stephen
commented
Aug 31, 2026
FE Regression Coverage ReportIncrement line coverage |
3950948 to
607f2cfCompareairborne12
commented
Sep 1, 2026
run buildall |
airborne12
commented
Sep 1, 2026
/review |
There was a problem hiding this comment.
Automated review status: request changes.
Critical checkpoints:
- Admission: startup/runtime config mutation and ordinary legacy/Nereids CREATE/ALTER reject V1 as intended.
- Existing-V1 compatibility: local journal replay, restore, reads/loads/compaction, and BUILD of committed indexes remain reachable, but synchronized/CCR CREATE and ADD INDEX for an existing explicit-V1 table are blocked before a trusted compatibility context can be honored.
- Config lifecycle: the new callback validates before assignment, but a rejected persistent HTTP update still reaches reset persistence and can erase unrelated overrides.
- Regression migration: the branch-only V1 success suite is fixed, remaining V1 DDL is rejection coverage, and changed golden query/result counts align. One additional converted reader fault suite is vacuous because its hook is V1-only; the two previously reported reader/writer hook issues were not duplicated.
- Existing discussions: the legacy DEFAULT sentinel, fixed single-replica suite, removed compaction-reader hook, and retained writer hook remain hard duplicate fences.
- User focus: no additional review focus was supplied.
- Convergence: two complete review rounds finished; every candidate was accepted, deduplicated, or dismissed with evidence, and all Round 2 reviewers reported NO_NEW_VALUABLE_FINDINGS.
| if (invertedIndexFileStorageFormat.equalsIgnoreCase("v1")) { | ||
| return TInvertedIndexFileStorageFormat.V1; | ||
| throw new AnalysisException( |
There was a problem hiding this comment.
[P1] Preserve synchronized replay of existing V1 tables
Env.getSyncedDdlStmt serializes an existing table's persisted inverted_index_storage_format together with is_being_synced=true, but the destination invokes this analyzer before synchronization-specific handling can run. An existing V1 table therefore now fails CCR/binlog CREATE replay at this throw, and synchronized ADD INDEX hits the same compatibility break in processAddIndex's exact-V1 guard. Please route trusted sync/CCR execution through a compatibility path that preserves explicit V1 (the SQL property alone is user-settable and cannot be trusted), and cover replay of an explicit-V1 CREATE and ADD INDEX.
There was a problem hiding this comment.
Verified against master and this is upstream-identical, not backport-specific: master PropertyAnalyzer has the exact same unconditional throw for v1 (lines 1240-1243 there vs 1293-1296 here, byte-identical text), and this PR ports that hunk verbatim.
So CCR/binlog replay of an existing explicit-V1 table hits the same rejection on master today. I am keeping the backport faithful rather than adding a 4.1-only sync/is_being_synced compatibility path, which would make the branches diverge on a user-visible behaviour.
That said, this is the most impactful of the findings for 4.1 users, since CCR replay of pre-existing V1 tables is a real upgrade scenario. Flagging it for the maintainers as a follow-up that should land on master first and then be picked here in the same shape. Leaving this thread open for that reason.
| "Default storage format of inverted index, the default value is V3." | ||
| }) | ||
| @ConfField(mutable = true, masterOnly = true, | ||
| callback = InvertedIndexStorageFormatValidator.RuntimeConfigHandler.class, |
There was a problem hiding this comment.
[P1] Keep a rejected V1 update from resetting persisted configs
For direct _set_config?inverted_index_storage_format=V1&persist=true, this callback rejects the only value, leaving setConfigs empty, but SetConfigAction still calls persistConfig(setConfigs, true). That reset path truncates fe_custom.conf, so a failed V1 update can erase every unrelated persisted override (and may expose a V1 value from fe.conf at the next startup). Please avoid reset persistence when validation produced errors, or make the request transactional, and add a test that the custom file is unchanged after this rejection.
There was a problem hiding this comment.
Same upstream-parity situation: master Config.java declares this field with the identical callback = InvertedIndexStorageFormatValidator.RuntimeConfigHandler.class, so the _set_config persist reset-persistence interaction you describe exists on master after #64522 as well. The only local adaptation in this hunk was keeping branch-4.1 bilingual description array (plus re-indentation for checkstyle); the callback wiring is verbatim.
Fixing the SetConfigAction / persistConfig interaction is a genuine improvement but it belongs on master first so both branches behave the same. Leaving this thread open for maintainer visibility.
Uh oh!
There was an error while loading. Please reload this page.
airborne12
commented
Sep 1, 2026
The P0 failure on build 1035495 ( |
airborne12
commented
Sep 1, 2026
The Baseline evidence: in the same time window three other unrelated PRs failed the identical way on this config — 1035655 (#67207, 16:33), 1035625 (#67125, 16:34), 1035660 (#67157, 16:40) — while builds just before and after that window (1035644, 1035627, 1035677, 1035678) all passed. Treating it as a known-good-elsewhere infra window; it will be re-run together with P0 once #67363 lands. |
airborne12
commented
Sep 2, 2026
run buildall |
607f2cf to
50590f6Compareairborne12
commented
Sep 3, 2026
run buildall |
1 similar comment
airborne12
commented
Sep 3, 2026
run buildall |
…4522 When users copy historical DDL that explicitly sets inverted index format to V1, new tables are silently created with V1, which lacks small-file merging and bypasses the packed file path, amplifying object-storage PUT requests. Block new inverted index V1 creation at FE (CREATE/ALTER and runtime config set), add a deprecation regression test, and migrate ~50 regression suites off V1. Conflicts: fe/fe-common/.../Config.java - branch-4.1 keeps the bilingual description array; merged it with the new RuntimeConfigHandler callback (ConfField callback mechanism exists in 4.1's ConfigBase). Re-indented for checkstyle. fe/fe-core/.../SchemaChangeHandler.java - kept 4.1's indexDef.getColumns() (getColumnNames() is master-only drift); inserted the V1-block check as-is. regression-test/... - test_nereids_show_{build_,}index.groovy live under nereids_p0/show/ on 4.1 (moved to query_p0/show/ on master); edits landed there. - test_index_change_6.groovy: adopted the PR's nonConcurrent tag, kept 4.1's timeout=60000 (master's 300000 is unrelated drift). - dropped V1 sections exactly as the source PR did in io_context / multi_match / index_change_6 / var_index. (cherry picked from commit d1070cb)
50590f6 to
ab11794Compareyiguolei
commented
Sep 5, 2026
run buildall |
hello-stephen
commented
Sep 5, 2026
FE Regression Coverage ReportIncrement line coverage |
Uh oh!
There was an error while loading. Please reload this page.
…roup (#67363) ### What problem does this PR solve? Issue Number: N/A Related PR: #67327 (which backported #65821 and introduced this suite) Problem Summary: `test_search_score_topn_predicates` fails intermittently on the shared P0 pipeline of branch-4.1. It always fails on its very first query (line 59), right after `CREATE TABLE` + `INSERT` + `sync`, with: ``` errCode = 2, detailMessage = [INTERNAL_ERROR]SearchExpr should not be executed without inverted index ``` Observed on branch-4.1 CI, 2 failures out of 4 runs — builds `1034808` (PR #67327) and `1035495` (PR #67342) failed, while `1035120` (rerun of #67327, identical code, 5530 tests passed) and `1035335` (PR #67340, after the suite landed) passed. At roughly a 50% hit rate it randomly reddens unrelated branch-4.1 PRs, so it needs to stop running in the concurrent P0 group. The case is not wrong on its own: it passes locally against both a 1-BE and a 3-BE cluster built from this branch, and the CI rerun with identical code passed as well. That points at contention on the shared P0 runner (the index of the freshly inserted rowset not being usable yet at query time) rather than at the query. branch-4.1 also lacks master's `prevent_search_row_fallback` path (added by #66052 together with the SNII format), so on this branch a search that does not take the index surfaces the internal error directly instead of falling back. Fix: move the suite to the `nonConcurrent` group so it runs serialized, as 35 other `inverted_index_p0` cases on this branch already do. No test logic or expected output changes. ### Release note None ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [x] Regression test - [ ] Unit Test - [ ] Manual test (add detailed scripts or steps below) - [ ] No need to test or manual test. Explain why: - [ ] This is a refactor/code format and no logic has been changed. - [ ] Previous test can cover this change. - [ ] No code files have been changed. - [ ] Other reason <!-- Add your reason? --> - Behavior changed: - [x] No. - [ ] Yes. <!-- Explain the behavior change --> - Does this need documentation? - [x] No. - [ ] Yes. <!-- Add document PR link here. eg: apache/doris-website#1214 --> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label <!-- Add branch pick label that this PR should merge into -->
What problem does this PR solve?
Issue Number: N/A
Related PR: #64522 (master), picked from commit d1070cb
Problem Summary:
Backport of #64522 to branch-4.1. When users copy historical DDL that explicitly sets inverted index format to V1, new tables are silently created with V1. V1 lacks small-file merging and bypasses the packed file path, significantly amplifying PUT requests to object storage. This change blocks new inverted index V1 creation at FE (CREATE TABLE / CREATE INDEX / ALTER, and runtime
ADMIN SET FRONTEND CONFIG), adds a V1-deprecation regression test, and migrates ~50 regression suites off V1.Hunk audit (source diff → this PR): 57 files total — 50 Ported (verbatim) (byte-identical change sets, incl. the new
InvertedIndexStorageFormatValidatorandtest_inverted_index_v1_deprecated), 7 Adapted:Config.javaRuntimeConfigHandlercallback (ConfFieldcallbackmechanism exists in 4.1'sConfigBase); re-indented for checkstyleSchemaChangeHandler.javaindexDef.getColumns()(getColumnNames()in the conflict context is master-only drift, not part of the source diff)test_nereids_show_build_index.groovy/test_nereids_show_index.groovynereids_p0/show/on 4.1 (moved toquery_p0/show/on master); the ±2 edits landed at the 4.1 pathstest_index_change_6.groovynonConcurrenttag; kept 4.1'stimeout = 60000(master's 300000 is unrelated drift); V1 section (-83 lines) removed as in sourcetest_index_io_context.groovy/test_index_multi_match.groovy/var_index.groovyLocal verification on this branch (ASAN build):
ConfigTest6/6,PropertyAnalyzerTest12/12 (both actually executed, verified in surefire output)test_inverted_index_v1_deprecated)Release note
Inverted index V1 is deprecated: creating new tables/indexes with
inverted_index_storage_format = V1(and setting it viaADMIN SET FRONTEND CONFIG) is now rejected. Existing V1 tables are unaffected.Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)