Skip to content

Conversation

@BugsGuru
Copy link
Collaborator

@BugsGuru BugsGuru commented Oct 22, 2025

User description

关联的 issue

fix: https://github.com/actiontech/sqle-ee/issues/2405

描述你的变更

修复MySQL规则:建议列与表使用同一个字符集

确认项(pr提交后操作)

Tip

请在指定复审人之前,确认并完成以下事项,完成后✅


  • 我已完成自测
  • 我已记录完整日志方便进行诊断
  • 我已在关联的issue里补充了实现方案
  • 我已在关联的issue里补充了测试影响面
  • 我已确认了变更的兼容性,如果不兼容则在issue里标记 not_compatible
  • 我已确认了是否要更新文档,如果要更新则在issue里标记 need_update_doc


Description

  • 使用表字符集与列字符集比较替换原有检查逻辑

  • 对 CREATE/ALTER TABLE 语句进行一致性校验

  • 校验过程中自动获取表的默认字符集

  • 更新测试用例确保多种字符集情况正确处理


Diagram Walkthrough

flowchart LR
  A["获取表字符集"]
  B["检查列字符集"]
  C["字符集一致"]
  D["字符集不一致"]
  A -- "通过表选项或默认值" --> B
  B -- "匹配" --> C
  B -- "不匹配" --> D
Loading

File Walkthrough

Relevant files
Bug fix
rule_00075.go
更新字符集一致性校验逻辑                                                                                         

sqle/driver/mysql/rule/ai/rule_00075.go

  • 更新规则提示描述说明新的校验逻辑
  • 从 CREATE 语句中获取表字符集
  • 对比列字符集与表字符集(忽略大小写)
  • 添加 ALTER TABLE 语句中字符集变更的处理逻辑
+54/-13 
Tests
rule_00075_test.go
更新测试用例以验证新字符集校验逻辑                                                                               

sqle/driver/mysql/rule_00075_test.go

  • 修改原测试用例中字符集校验场景
  • 增加对不同字符集组合的测试
  • 更新期望结果以匹配新逻辑
+51/-33 

@github-actions
Copy link

github-actions bot commented Oct 22, 2025

PR Reviewer Guide 🔍

(Review updated until commit dbb088e)

🎫 Ticket compliance analysis 🔶

2405 - Partially compliant

Compliant requirements:

  • 使用表字符集与列字符集比较替换原有检查逻辑
  • 更新测试用例覆盖多种字符集场景

Non-compliant requirements:

Requires further human verification:

  • 确认在未明确指定表字符集时,从 schema 默认获取字符集的逻辑是否在所有场景均正确工作
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

字符集校验

请检查对表字符集的获取和后续比较逻辑是否在所有边界情况(例如未指定或默认字符集)下均能正确工作,并确保错误处理及时返回异常。

var tableCharset string
var err error

switch stmt := input.Node.(type) {
case *ast.CreateTableStmt:
	// Get table charset from CREATE TABLE statement
	if charsetOption := util.GetTableOption(stmt.Options, ast.TableOptionCharset); charsetOption != nil {
		tableCharset = charsetOption.StrValue
	} else {
		// If no table charset specified, get from schema default
		tableCharset, err = input.Ctx.GetSchemaCharacter(stmt.Table, "")
		if err != nil {
			return err
		}
	}
测试覆盖

请确认新增测试用例是否涵盖了 CREATE TABLE 与 ALTER TABLE 的所有字符集变化场景,以验证字符集一致性校验方法的完整性。

//create table, no table charset, no column charset - should pass
runSingleRuleInspectCase(rule, t, "create table, no table charset, no column charset", DefaultMysqlInspect(), `
CREATE TABLE  if not exists exist_db.not_exist_tb_1 (
id bigint unsigned DEFAULT 100 AUTO_INCREMENT,
a varchar(10),
PRIMARY KEY (id)
);
`, newTestResult())

//create table, with table charset utf8mb4, no column charset - should pass
runSingleRuleInspectCase(rule, t, "create table, with table charset utf8mb4, no column charset", DefaultMysqlInspect(), `
CREATE TABLE  if not exists exist_db.not_exist_tb_1 (
id bigint unsigned DEFAULT 100 AUTO_INCREMENT,
a varchar(10),
PRIMARY KEY (id)
) CHARSET utf8mb4;
`, newTestResult())

//create table, with table charset utf8mb4, column charset utf8mb4 - should pass
runSingleRuleInspectCase(rule, t, "create table, with table charset utf8mb4, column charset utf8mb4", DefaultMysqlInspect(), `
CREATE TABLE  if not exists exist_db.not_exist_tb_1 (
id bigint unsigned DEFAULT 100 AUTO_INCREMENT,
a varchar(10) CHARSET utf8mb4,
PRIMARY KEY (id)
) CHARSET utf8mb4;
`, newTestResult())

//create table, with table charset utf8mb4, column charset utf8 - should fail
runSingleRuleInspectCase(rule, t, "create table, with table charset utf8mb4, column charset utf8", DefaultMysqlInspect(), `
CREATE TABLE  if not exists exist_db.not_exist_tb_1 (
id bigint unsigned DEFAULT 100 AUTO_INCREMENT,
a varchar(10) CHARSET utf8,
PRIMARY KEY (id)
) CHARSET utf8mb4;
`, newTestResult().addResult(ruleName, "a"))

//create table, with table charset utf8, column charset utf8mb4 - should fail
runSingleRuleInspectCase(rule, t, "create table, with table charset utf8, column charset utf8mb4", DefaultMysqlInspect(), `
CREATE TABLE  if not exists exist_db.not_exist_tb_1 (
id bigint unsigned DEFAULT 100 AUTO_INCREMENT,
a varchar(10) CHARSET utf8mb4,
PRIMARY KEY (id)
) CHARSET utf8;
`, newTestResult().addResult(ruleName, "a"))

//alter table add column, no table charset change, no column charset - should pass
runSingleRuleInspectCase(rule, t, "alter table add column, no table charset change, no column charset", DefaultMysqlInspect(), `
ALTER TABLE exist_db.exist_tb_1 ADD COLUMN a varchar(10) COMMENT "unit test";
`, newTestResult())

//alter table add column, no table charset change, column charset utf8mb4 - should pass (assuming table charset is utf8mb4)
runSingleRuleInspectCase(rule, t, "alter table add column, no table charset change, column charset utf8mb4", DefaultMysqlInspect(), `
ALTER TABLE exist_db.exist_tb_1 ADD COLUMN a varchar(10) CHARSET utf8mb4 COMMENT "unit test";
`, newTestResult())

//alter table add column, change table charset to utf8, column charset utf8mb4 - should fail
runSingleRuleInspectCase(rule, t, "alter table add column, change table charset to utf8, column charset utf8mb4", DefaultMysqlInspect(), `
ALTER TABLE exist_db.exist_tb_1 ADD COLUMN a varchar(10) CHARSET utf8mb4 COMMENT "unit test", CONVERT TO CHARACTER SET utf8;
`, newTestResult().addResult(ruleName, "a"))

//alter table modify column, no table charset change, no column charset - should pass
runSingleRuleInspectCase(rule, t, "alter table modify column, no table charset change, no column charset", DefaultMysqlInspect(), `
ALTER TABLE exist_db.exist_tb_1 MODIFY v1 varchar(10) COMMENT "unit test";
`, newTestResult())

//alter table modify column, no table charset change, column charset utf8mb4 - should pass (assuming table charset is utf8mb4)
runSingleRuleInspectCase(rule, t, "alter table modify column, no table charset change, column charset utf8mb4", DefaultMysqlInspect(), `
ALTER TABLE exist_db.exist_tb_1 MODIFY v1 varchar(10) CHARSET utf8mb4 COMMENT "unit test";
`, newTestResult())

//alter table modify column, change table charset to utf8, column charset utf8mb4 - should fail
runSingleRuleInspectCase(rule, t, "alter table modify column, change table charset to utf8, column charset utf8mb4", DefaultMysqlInspect(), `
ALTER TABLE exist_db.exist_tb_1 MODIFY v1 varchar(10) CHARSET utf8mb4 COMMENT "unit test", CONVERT TO CHARACTER SET utf8;
`, newTestResult().addResult(ruleName, "v1"))

//alter table change column, no table charset change, no column charset - should pass
runSingleRuleInspectCase(rule, t, "alter table change column, no table charset change, no column charset", DefaultMysqlInspect(), `
ALTER TABLE exist_db.exist_tb_1 CHANGE COLUMN v1 a varchar(10) COMMENT "unit test";
`, newTestResult())

//alter table change column, no table charset change, column charset utf8mb4 - should pass (assuming table charset is utf8mb4)
runSingleRuleInspectCase(rule, t, "alter table change column, no table charset change, column charset utf8mb4", DefaultMysqlInspect(), `
ALTER TABLE exist_db.exist_tb_1 CHANGE COLUMN v1 a varchar(10) CHARSET utf8mb4 COMMENT "unit test";
`, newTestResult())

//alter table change column, change table charset to utf8, column charset utf8mb4 - should fail
runSingleRuleInspectCase(rule, t, "alter table change column, change table charset to utf8, column charset utf8mb4", DefaultMysqlInspect(), `
ALTER TABLE exist_db.exist_tb_1 CHANGE COLUMN v1 a varchar(10) CHARSET utf8mb4 COMMENT "unit test", CONVERT TO CHARACTER SET utf8;
`, newTestResult().addResult(ruleName, "a"))

@github-actions
Copy link

github-actions bot commented Oct 22, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
修改字符集覆盖逻辑

目前在处理 ALTER TABLE 语句时,注释要求使用最后指定的字符集,但内层循环中的 break
会在匹配到第一个字符集选项后退出,可能导致后续覆盖的选项被忽略。建议移除或调整 break,以确保最后一个指定的字符集被正确使用。

sqle/driver/mysql/rule/ai/rule_00075.go [94-104]

 for _, spec := range stmt.Specs {
-		// Use the last defined table character set
 		if spec.Tp == ast.AlterTableOption {
 			for _, option := range spec.Options {
 				if option.Tp == ast.TableOptionCharset {
 					tableCharset = option.StrValue
-					break
+					// 不退出循环,确保最后出现的字符集生效
 				}
 			}
 		}
 	}
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the early break prevents processing all charset options, potentially ignoring later overrides, and recommends removing it to honor the comment about using the last defined charset.

Medium

@github-actions
Copy link

Persistent review updated to latest commit dbb088e

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
修改字符集覆盖逻辑

目前在处理 ALTER TABLE 语句时,注释要求使用最后指定的字符集,但内层循环中的 break
会在匹配到第一个字符集选项后退出,可能导致后续覆盖的选项被忽略。建议移除或调整 break,以确保最后一个指定的字符集被正确使用。

sqle/driver/mysql/rule/ai/rule_00075.go [94-104]

 for _, spec := range stmt.Specs {
-		// Use the last defined table character set
 		if spec.Tp == ast.AlterTableOption {
 			for _, option := range spec.Options {
 				if option.Tp == ast.TableOptionCharset {
 					tableCharset = option.StrValue
-					break
+					// 不退出循环,确保最后出现的字符集生效
 				}
 			}
 		}
 	}
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the early break prevents processing all charset options, potentially ignoring later overrides, and recommends removing it to honor the comment about using the last defined charset.

Medium

@iwanghc iwanghc merged commit c67969c into main Oct 22, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants