fix: honor if exists for missing drop index - #25006
Merged
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
7 tasks
heni02
approved these changes
Jun 16, 2026
XuPeng-SH
approved these changes
Jun 17, 2026
XuPeng-SH
left a comment
XuPeng-SH
left a comment
Contributor
There was a problem hiding this comment.
🔍 Multi-Angle Review Summary
经过全面review，从以下角度分析了这个PR：
- ✅ Core logic correctness (核心逻辑正确性)
- ✅ Concurrency safety (并发安全)
- ✅ Error handling (错误处理)
- ✅ MySQL compatibility (MySQL兼容性)
- 🟡 Test coverage (测试覆盖 - 建议补充)
- ✅ Unhappy paths (异常路径)
✅ 核心逻辑
实现方式
使用空字符串sentinel表示no-op路径：
Planner:
if !found {
if stmt.IfExists {
dropIndex.IndexName = "" // Sentinel for no-op
} else {
return nil, moerr.NewInternalErrorf(...)
}
}Compiler:
if qry.GetIndexName() == "" {
return nil // Early return for no-op
}评估
- ✅ 逻辑正确: Early return避免不必要的执行
- ✅ 最小改动: 只在必要位置添加检查
- ✅ 职责分离: Planner判断，Compiler执行
- 🟡 设计选择: 空字符串不是最explicit的方式，但有清晰注释且实际可行
✅ Unhappy Path Analysis
已测试场景 ✅:
- Index不存在 -
drop index if exists idx1; drop index if exists idx1;→ 第二次成功 - 无IF EXISTS -
drop index nonexist;→ 报错 ✅
未测试但理论正确的场景 🟡:
-
Table不存在 -
DROP INDEX IF EXISTS idx1 ON nonexist_table;- 期望: 成功（因为early return在table check之前）
- 建议: 添加test验证
-
Database不存在 -
DROP INDEX IF EXISTS idx1 ON nonexist_db.t1;- 期望: 成功（同样early return）
- 建议: 添加test验证
-
并发场景 - 多个session同时drop不存在的index
- 期望: 都成功
- 建议: 添加并发test
✅ MySQL Compatibility
MySQL:
mysql> DROP INDEX IF EXISTS nonexist ON t1;
Query OK, 0 rows affected (0.00 sec)
mysql> DROP INDEX nonexist ON t1;
ERROR 1091 (42000): Can't DROP 'nonexist'; check that...MatrixOne (此PR):
mo> DROP INDEX IF EXISTS nonexist ON t1;
Query OK ✅
mo> DROP INDEX nonexist ON t1;
ERROR: not found index: nonexist ✅结论: ✅ 语义兼容
✅ 代码质量
优点:
- ✅ 注释清晰，解释sentinel语义
- ✅ Early return避免复杂执行路径
- ✅ 测试覆盖基本场景
- ✅ 向后兼容
建议改进 (Non-blocking):
- 🟡 P1: 添加edge case tests (database/table不存在)
- 🟢 P2: 添加并发测试
- 🟢 P2: Future improvement: 考虑用explicit flag替代empty sentinel
✅ Test Coverage
已覆盖:
- ✅ Unit test:
TestDropIndexIfExistsMissingIndex - ✅ Compile test:
TestTableScopedDDLDatabaseEOBMapsToNoSuchTable - ✅ BVT:
drop_if_exists.sql(连续drop)
建议添加:
-- Edge case 1: table不存在
DROP INDEX IF EXISTS idx1 ON nonexist_table;
-- Edge case 2: database不存在
DROP INDEX IF EXISTS idx1 ON nonexist_db.t1;📊 Decision: APPROVE ✅
无Blocking Issues
理由:
- ✅ 核心逻辑正确
- ✅ 无并发安全问题
- ✅ 错误处理合理
- ✅ MySQL语义兼容
- ✅ 测试覆盖主要场景
- 🟡 建议补充edge case tests (non-blocking)
这是一个clean, minimal, correct的bug fix! 👍
建议在后续PR中补充edge case和并发测试。
详细分析见session artifacts。
Contributor
🟡 建议补充的测试场景 (Non-blocking)虽然当前测试已覆盖主要场景，建议后续添加以下edge cases: 1. Table不存在的情况-- test/distributed/cases/ddl/drop_if_exists.sql
use db1;
DROP INDEX IF EXISTS idx1 ON nonexist_table; -- 应该成功2. Database不存在的情况DROP INDEX IF EXISTS idx1 ON nonexist_db.t1; -- 应该成功3. 补充单元测试// pkg/sql/plan/build_test.go
func TestDropIndexIfExistsWithNonexistentTable(t *testing.T) {
mock := NewMockOptimizer(true)
// 这个应该成功（table不存在，但有IF EXISTS）
_, err := runOneStmt(mock, t, "drop index if exists idx1 on nonexist_table")
require.NoError(t, err)
}这些场景理论上都会成功（因为early return在database/table check之前），但添加explicit tests可以：
当前PR已经是一个clean的bug fix，这些是future improvement建议，不影响approve! 👍 |
aunjgr
approved these changes
Jun 17, 2026
Contributor
Merge Queue Status
This pull request spent 1 hour 4 minutes 47 seconds in the queue, including 1 hour 4 minutes 29 seconds running CI. Required conditions to merge
|
mergify Bot
pushed a commit
that referenced
this pull request
Jun 17, 2026
Cherry-pick #25006 to `4.0-dev`. - Treat missing indexes as a no-op for `DROP INDEX IF EXISTS` instead of returning `not found index`. - Keep the existing error behavior for `DROP INDEX` without `IF EXISTS`. - Add planner, compile, and BVT coverage for the missing-index path. Validation: - `gofmt` - `git show --check --oneline HEAD` - `go test ./pkg/sql/plan` and `go test ./pkg/sql/compile` were blocked locally by missing C headers: `usearch.h` and `xxhash.h`. - Focused mo-tester was not repeated for this cherry-pick because the same local flow hung during cleanup without producing a fresh report while validating #25006. Approved by: @heni02, @XuPeng-SH, @aunjgr
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
5 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 type of PR is this?
Which issue(s) this PR fixes:
issue #24821
What this PR does / why we need it:
DROP INDEX IF EXISTSinstead of returningnot found index.DROP INDEXwithoutIF EXISTS.Validation:
gofmtgit diff --checkgo test ./pkg/sql/planandgo test ./pkg/sql/compilewere blocked locally by missing C headers:usearch.handxxhash.h.drop_if_existswas attempted, but mo-tester hung during cleanup without producing a fresh report, so the process was terminated.