Uh oh!
There was an error while loading. Please reload this page.
[fix](fe) block inverted index V1 creation in FE - #64522
Conversation
hello-stephen
commented
Jun 15, 2026
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
airborne12
commented
Jun 17, 2026
run buildall |
airborne12
commented
Jun 17, 2026
/review |
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
There was a problem hiding this comment.
I am requesting changes. The analyzer now rejects an explicit inverted_index_storage_format = v1, but it does not yet block all new V1 creation paths, and the regression coverage around V1 is incomplete.
Critical checkpoint conclusions:
- Goal/test proof: partially satisfied only for explicit V1 table properties. New V1 creation remains possible through the default/config path, and compatibility coverage is not deterministic.
- Scope/focus: the code change is small, but related test updates are incomplete because existing V1 regression suites are left failing or inconsistent with the new policy.
- Concurrency/lifecycle: not involved in this PR.
- Config: involved.
Config.inverted_index_storage_formatis mutable and can still drive new V1 table/index creation. - Compatibility: legacy metadata loading appears to stay on
TablePropertyrather than this analyzer, but read/write compatibility for existing V1 is not actually proven by the new regression block. - Parallel paths: create-table default/config paths and existing regression V1 setup paths need to be handled.
- Tests: explicit rejection is covered, but the unit test does not assert the exception message, the AC4 block can pass after real failures, and existing p0 suites still create V1 tables.
- Observability, transactions, persistence, FE-BE protocol: no new concerns found for this scoped FE analyzer change.
- User focus: no additional user-provided focus points were supplied.
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.
Uh oh!
There was an error while loading. Please reload this page.
hello-stephen
commented
Jun 17, 2026
TPC-H: Total hot run time: 28871 ms |
hello-stephen
commented
Jun 17, 2026
TPC-DS: Total hot run time: 175335 ms |
hello-stephen
commented
Jun 17, 2026
ClickBench: Total hot run time: 25.27 s |
hello-stephen
commented
Jun 17, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Jun 17, 2026
FE Regression Coverage ReportIncrement line coverage |
c5625f0 to
926aac4Compareairborne12
commented
Jun 18, 2026
/review |
There was a problem hiding this comment.
Review opinion: request changes.
I found one functional gap and several regression-test issues introduced by the PR updates. I did not duplicate the existing inline threads about the original default/config V1 path, assertThrows message check, broad residual V1 suite failures, or the earlier non-failing legacy fixture.
Critical checkpoint conclusions:
- Goal and proof: Not fully met. Explicit CREATE TABLE rejection is covered, but ALTER TABLE ADD INDEX on an existing V1 table can still create a new V1 index, and several updated tests now fail, do nothing, or provide dead coverage.
- Scope and focus: The FE change is small, but the regression edits are broader than needed and comment out unrelated assertions.
- Concurrency and lifecycle: No new concurrency or lifecycle-management risk found in the touched code.
- Config: A mutable master-only V1 creation escape hatch was added for tests; production blocking still depends on admins not enabling it.
- Compatibility and parallel paths: Existing V1 metadata loading is preserved, but the schema-change add-index path was not updated alongside table-property analysis.
- Conditional checks and error handling: The explicit V1 error is clear; no exception-boundary issue found in the changed FE code.
- Tests and results: Current test coverage has regressions: ANN V1 negative coverage was changed into a V2 success path, the alter-format suite is now a no-op, a V1-only debug point is exercised only with V2, and a variant subtype negative case was disabled.
- Observability, transaction/persistence, FE-BE variable passing, and performance: No additional issues found beyond the new V1-index creation path.
- User focus: No additional user-provided review focus was supplied.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
1717801 to
c9a2c04Compare9c94978 to
052ed42CompareUh oh!
There was an error while loading. Please reload this page.
airborne12
left a comment
There was a problem hiding this comment.
Requesting changes for the V1 deprecation semantics and compatibility coverage issues called out inline.
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.
Uh oh!
There was an error while loading. Please reload this page.
cc4ae77 to
cec3caaCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
008aee3 to
0ce59aaCompare3142fdb to
0d6db7fCompareairborne12
commented
Aug 21, 2026
run buildall |
hello-stephen
commented
Aug 21, 2026
TPC-H: Total hot run time: 17394 ms |
hello-stephen
commented
Aug 21, 2026
TPC-DS: Total hot run time: 83734 ms |
hello-stephen
commented
Aug 21, 2026
ClickBench: Total hot run time: 14.66 s |
hello-stephen
commented
Aug 21, 2026
FE UT Coverage ReportIncrement line coverage |
hello-stephen
commented
Aug 21, 2026
FE Regression Coverage ReportIncrement line coverage |
Uh oh!
There was an error while loading. Please reload this page.
…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)
IcebergWritePlanProviderTest asserts baseSnapshotId == null for an explicitly pinned empty read, but apache#66348 intentionally changed this to keep the pinned -1 as an OCC fence; the test from apache#66345 was not updated. PropertyAnalyzerTest expects the partition-level "only supports V2, V3 and SNII" message for V1, but apache#64522 already throws the "deprecated" message inside analyzeInvertedIndexFileStorageFormat before the partition-level check can run, making the expected branch unreachable. Update the Iceberg assertion to expect -1 (matching the intent documented in apache#66348), and accept either V1-rejection message in the PropertyAnalyzer assertion. Test-only, no production code changed.
…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)
…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)
…67342) ### 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 `InvertedIndexStorageFormatValidator` and `test_inverted_index_v1_deprecated`), **7 Adapted**: | File | Adaptation | |---|---| | `Config.java` | 4.1 keeps the bilingual description array; merged with the new `RuntimeConfigHandler` callback (ConfField `callback` mechanism exists in 4.1's `ConfigBase`); re-indented for checkstyle | | `SchemaChangeHandler.java` | the +7-line V1-block check ported verbatim; kept 4.1's `indexDef.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.groovy` | these suites live under `nereids_p0/show/` on 4.1 (moved to `query_p0/show/` on master); the ±2 edits landed at the 4.1 paths | | `test_index_change_6.groovy` | adopted the PR's `nonConcurrent` tag; kept 4.1's `timeout = 60000` (master's 300000 is unrelated drift); V1 section (-83 lines) removed as in source | | `test_index_io_context.groovy` / `test_index_multi_match.groovy` / `var_index.groovy` | V1 tables/sections removed exactly as in source; post-resolution V1-identifier sweep matches master's files 1:1 | **Local verification on this branch (ASAN build)**: - FE UT: `ConfigTest` 6/6, `PropertyAnalyzerTest` 12/12 (both actually executed, verified in surefire output) - Regression: all 46 suites touched by this PR (23 inverted_index_p0, 7 fault_injection_p0, 6 variant_p0, 2 nereids_p0/show, 1 fault_injection_p2, plus ann_index/correctness/datatype/function/insert_overwrite/nereids_arith/variant_mv) ran against a local 1FE+1BE cluster built from this PR — **46/46 passed, 0 failed** (incl. the new `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 via `ADMIN SET FRONTEND CONFIG`) is now rejected. Existing V1 tables are unaffected. ### Check List (For Author) - Test <!-- At least one of them must be included. --> - [x] Regression test - [x] 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: - [ ] No. - [x] Yes. <!-- Explain the behavior change --> New inverted index V1 creation is rejected at FE (same behavior as master after #64522). - 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 --> Co-authored-by: liangj777 <106017102+LIANG751234313@users.noreply.github.com>
What problem does this PR solve?
Problem Summary:
When users copy historical DDL statements that explicitly set inverted index format to V1, new tables are silently created with V1. Inverted index V1 does not support small file merging and bypasses the packed file path, significantly amplifying PUT requests to object storage.
Observed impact: clusters with high write QPS are seeing300–400 PUT/s, with 70–80% attributed to inverted index V1.
Block new inverted index V1 creation at the FE level:
inverted_index_storage_format = v1on a new indexnow returns an error immediately.are unaffected. No forced migration.
Error message:Inverted index V1 is deprecated and no longer allowed for new index creation.Please use inverted index V2.
Coverage
CREATE TABLEwith inline index definitionALTER TABLE ADD INDEXRelease note
Check List (For Author)
Test
Added regression-test/suites/inverted_index_p0/storage_format/test_inverted_index_v1_deprecated.groovy
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)