branch-4.1: [fix](fe) block inverted index V1 creation in FE #64522 - #67342
branch-4.1: [fix](fe) block inverted index V1 creation in FE #64522#67342airborne12 wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
run compile |
1 similar comment
|
run compile |
|
/review |
|
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. |
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
FE Regression Coverage ReportIncrement line coverage |
3950948 to
607f2cf
Compare
|
run buildall |
|
/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.
|
The P0 failure on build 1035495 ( |
|
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. |
|
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)
607f2cf to
50590f6
Compare
|
run buildall |
1 similar comment
|
run buildall |
3 participants
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)