add alter partition support

[zhangxu19830126]

User description

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

issue #16438

What this PR does / why we need it:

support alter table add/drop/truncate/redefine partitions

---

PR Type

Enhancement

---

Description

• Add comprehensive ALTER TABLE partition support

• Implement ADD, DROP, TRUNCATE, and REDEFINE operations

• Add partition service methods and storage layer

• Include comprehensive test coverage for operations

---

Diagram Walkthrough

flowchart LR
  A["ALTER TABLE Statement"] --> B["Partition Service"]
  B --> C["Storage Layer"]
  C --> D["Metadata Updates"]
  C --> E["Partition Tables"]
  B --> F["ADD Partitions"]
  B --> G["DROP Partitions"] 
  B --> H["TRUNCATE Partitions"]
  B --> I["REDEFINE Partitions"]

Loading

File Walkthrough

	Relevant files
Enhancement	8 files storage.go Add partition storage operations implementation                    +462/-1  service.go Implement partition service ALTER methods                                +343/-11 alter.go Add ALTER partition compilation support                                    +125/-33 types.go Define partition service interface methods                              +75/-1    build_ddl.go Add ALTER partition plan building                                                +18/-6    service_range.go Refactor expression handling for range partitions                +1/-9      service_list.go Refactor expression handling for list partitions                  +1/-9      plan.proto Add ALTER partition protobuf definitions                                  +12/-0
Tests	3 files alter_parittion_test.go Add comprehensive ALTER partition tests                                    +342/-0  storage_test.go Add mock storage methods for testing                                          +50/-0    partition_test.go Fix partition count assertion in tests                                      +1/-1
Bug fix	2 files alter.go Fix memory management in ALTER clauses                                      +5/-5      build_show_util.go Fix context usage in partition metadata                                    +1/-1
Additional files	1 files plan.pb.go +1103/-840

---

[qodo-merge-pro]

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 Security concerns SQL construction: Multiple places build SQL via fmt.Sprintf with identifiers and literals (e.g., DROP/TRUNCATE/RENAME, UPDATE statements). While inputs are internal metadata, partition names and table names may originate from user DDL. Consider using parameterized execution or strict identifier quoting/escaping to avoid injection or malformed SQL.

⚡ Recommended focus areas for review Possible Issue In DropPartitions, the constructed IN clause string ends with a trailing comma and is sliced without guarding for empty input; additionally, partition names are interpolated directly without escaping, which can break SQL if names contain quotes or special chars. res, err := txn.Exec( fmt.Sprintf("delete from %s where primary_table_id = %d and partition_name in (%s)", catalog.MOPartitionTables, metadata.TableID, whereCause[:len(whereCause)-1], ), executor.StatementOption{}, ) if err != nil { Logic Risk When qry.AlterPartition is nil and AlgorithmType is not COPY, the code expects RawSQL to be present or handles only rename; otherwise it panics. This can cause user-visible panics for valid alter cases lacking RawSQL. Consider graceful error handling or guaranteed RawSQL presence. } panic("missing RawSQL for alter partition tables") } metadata, err := ps.GetPartitionMetadata( c.proc.Ctx, qry.TableDef.TblId, Validation Missing AddPartitions currently lacks checks for overlapping ranges/list values or duplicate names beyond TODOs. This can allow inconsistent partition metadata. Ensure validation before persisting changes. switch metadata.Method { case partition.PartitionMethod_Hash, partition.PartitionMethod_Key, partition.PartitionMethod_LinearHash, partition.PartitionMethod_LinearKey: return moerr.NewNotSupportedNoCtx("add partition is not supported for hash/key partitioned table") case partition.PartitionMethod_Range: // TODO: check overlapping range case partition.PartitionMethod_List: // TODO: check overlapping list values default: panic("BUG: unsupported partition method") } var values []partition.Partition n := len(metadata.Partitions) for i, p := range partitions { values = append(values, partition.Partition{ Name: p.Name.String(), PartitionTableName: GetPartitionTableName(def.Name, p.Name.String()), Position: uint32(i + n), ExprStr: getExpr(p), Expr: newTestValuesInExpr2(p.Name.String()), }, )

[qodo-merge-pro]

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Complex partition operations lack proper validation The PR's new partition operations (ADD, DROP, etc.) are missing crucial validation. Specifically, it fails to check for overlapping partitions when adding new ones and doesn't prevent dropping the last partition, risking data corruption and invalid table states. Examples: pkg/partitionservice/service.go [214-217] // TODO: check overlapping range case partition.PartitionMethod_List: // TODO: check overlapping list values pkg/partitionservice/storage.go [537-553] whereCause := "" tables := make([]string, 0, len(partitions)) for _, p := range partitions { found := false for i, mp := range metadata.Partitions { if p == mp.Name { metadata.Partitions = append(metadata.Partitions[:i], metadata.Partitions[i+1:]...) whereCause += fmt.Sprintf("'%s',", p) tables = append(tables, mp.PartitionTableName) found = true ... (clipped 7 lines) Solution Walkthrough: Before: // In pkg/partitionservice/service.go func (s *Service) AddPartitions(...) error { // ... switch metadata.Method { // ... case partition.PartitionMethod_Range: // TODO: check overlapping range case partition.PartitionMethod_List: // TODO: check overlapping list values } // ... return s.store.AddPartitions(...) } // In pkg/partitionservice/storage.go func (s *Storage) DropPartitions(...) error { // ... // No check to ensure at least one partition remains after dropping. // The table could be left with zero partitions. return s.exec.ExecTxn(...) } After: // In pkg/partitionservice/service.go func (s *Service) AddPartitions(...) error { // ... // Validate for overlapping partitions before adding if hasOverlappingPartitions(metadata.Partitions, newPartitions) { return moerr.NewInvalidInput("new partitions overlap with existing ones") } switch metadata.Method { // ... } // ... return s.store.AddPartitions(...) } // In pkg/partitionservice/storage.go func (s *Storage) DropPartitions(...) error { if len(metadata.Partitions) <= len(partitions) { return moerr.NewInvalidInput("cannot drop all partitions of a table") } // ... return s.exec.ExecTxn(...) } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies critical missing validation for partition operations, such as checking for overlapping values in ADD PARTITION and ensuring at least one partition remains in DROP PARTITION, which could lead to data integrity issues.	High
Possible issue	Fix early return in loop Move the return nil statement out of the for loop in the Rename function to prevent an early exit and ensure all partitions are processed. Additionally, relocate the metadata update for table_name to after the loop, as it only needs to be executed once. pkg/partitionservice/storage.go [401-447] for _, p := range metadata.Partitions { txn.Use(metadata.DatabaseName) res, err := txn.Exec( fmt.Sprintf( "rename table `%s` to `%s`", p.PartitionTableName, GetPartitionTableName(newName, p.Name), ), executor.StatementOption{}. WithIgnoreForeignKey(). WithIgnorePublish(), ) if err != nil { return err } res.Close() txn.Use(catalog.MO_CATALOG) res, err = txn.Exec( fmt.Sprintf("update %s set partition_table_name = '%s' where partition_table_name = '%s' and primary_table_id = %d", catalog.MOPartitionTables, GetPartitionTableName(newName, p.Name), p.PartitionTableName, metadata.TableID, ), executor.StatementOption{}, ) if err != nil { return err } res.Close() - - res, err = txn.Exec( - fmt.Sprintf("update %s set table_name = '%s' where table_id = %d", - catalog.MOPartitionMetadata, - newName, - metadata.TableID, - ), - executor.StatementOption{}, - ) - if err != nil { - return err - } - res.Close() - - return nil } +txn.Use(catalog.MO_CATALOG) +res, err := txn.Exec( + fmt.Sprintf("update %s set table_name = '%s' where table_id = %d", + catalog.MOPartitionMetadata, + newName, + metadata.TableID, + ), + executor.StatementOption{}, +) +if err != nil { + return err +} +res.Close() + +return nil + Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical bug where an early return nil inside a for loop would cause the function to exit after only one iteration, leaving the renaming operation incomplete.	High
General	Replace SELECT * with explicit columns *In the Redefine function, replace SELECT with an explicit list of column names in the INSERT ... SELECT statement to prevent potential issues from schema changes. pkg/partitionservice/storage.go [337-340] -sql = fmt.Sprintf("insert into `%s` select * from `%s`", +columnNames := make([]string, 0, len(def.Cols)) +for _, col := range def.Cols { + columnNames = append(columnNames, fmt.Sprintf("`%s`", col.Name)) +} +columns := strings.Join(columnNames, ", ") +sql = fmt.Sprintf("insert into `%s` (%s) select %s from `%s`", tmp, + columns, + columns, def.Name, ) Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that using SELECT * is fragile and recommends replacing it with an explicit column list, which improves the robustness and maintainability of the data migration logic.	Low
	Remove hardcoded test function from production code Remove the test-specific function newTestValuesInExpr2 with its hardcoded values from the production service.go file. pkg/partitionservice/service.go [530-606] -func newTestValuesInExpr2(col string) *plan.Expr { +func newValuesInExpr(col string, values []int64) *plan.Expr { + valueExprs := make([]*plan.Expr, 0, len(values)) + for _, val := range values { + valueExprs = append(valueExprs, &plan.Expr{ + Typ: plan.Type{Id: 22}, + Expr: &plan.Expr_Lit{ + Lit: &plan.Literal{ + Value: &plan.Literal_I64Val{I64Val: val}, + }, + }, + }) + } + return &plan.Expr{ Typ: plan.Type{Id: 10}, Expr: &plan.Expr_F{ F: &plan.Function{ - Func: &plan.ObjectRef{ - ObjName: "in", - Obj: 506806140934, - }, + Func: &plan.ObjectRef{ObjName: "in"}, Args: []*plan.Expr{ { Typ: plan.Type{Id: 22}, Expr: &plan.Expr_Col{ - Col: &plan.ColRef{RelPos: 1, ColPos: 0, Name: col}, + Col: &plan.ColRef{Name: col}, }, }, - ... + { + Typ: plan.Type{Id: 202}, + Expr: &plan.Expr_List{ + List: &plan.ExprList{List: valueExprs}, + }, + }, }, }, }, } } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: The suggestion correctly points out that newTestValuesInExpr2 is test-specific code with magic numbers that should not be in production code, which is a valid concern for maintainability and correctness.	Low
	Restore cache functionality or remove dead code Restore the caching logic in readMetadata or remove the now-unused metadataCache struct and related fields to eliminate dead code. pkg/partitionservice/service.go [443-461] func (s *Service) readMetadata( ctx context.Context, tableID uint64, txnOp client.TxnOperator, ) (partition.PartitionMetadata, error) { - // TODO: use cache + s.mu.RLock() + c, ok := s.mu.tables[tableID] + s.mu.RUnlock() + if ok { + return c.metadata, nil + } + metadata, ok, err := s.store.GetMetadata( ctx, tableID, txnOp, ) if err != nil { return partition.PartitionMetadata{}, err } if !ok { return partition.PartitionMetadata{}, nil } + + s.mu.Lock() + s.mu.tables[tableID] = newMetadataCache(metadata) + s.mu.Unlock() return metadata, nil } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that caching logic was removed, but it's a temporary change indicated by a TODO comment, making this a valid but non-critical observation about code consistency.	Low
More