Skip to content

lint: evaluate schema linters against post-state #840

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
morgo merged 2 commits into from
May 12, 2026
Merged

lint: evaluate schema linters against post-state #840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9917fa
lint: evaluate schema linters against post-state to fix conversion fa…
morgo May 12, 2026
9ea6f75
lint(has_float): restore "New column" message wording for ADD COLUMN
morgo May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 38 additions & 44 deletions pkg/lint/lint_has_float.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ package lint

import (
"fmt"
"strings"

"github.com/block/spirit/pkg/statement"
"github.com/pingcap/tidb/pkg/parser/ast"
"github.com/pingcap/tidb/pkg/parser/mysql"
)

Expand All @@ -26,58 +26,52 @@ func (l *HasFloatLinter) Description() string {
return "Detects usage of FLOAT or DOUBLE data types in table definitions"
}

// Lint operates on a post-state view of the schema, so a column being
// converted away from FLOAT/DOUBLE in an ALTER doesn't generate a false
// positive. The violation message uses three forms to preserve
// actionability:
//
// - "New column …" when the column did not exist in the pre-state
// (ADD COLUMN, or a column in a CREATE TABLE).
// - "Column … modified to use …" when the column existed pre-state and
// is being retyped by MODIFY / CHANGE COLUMN.
// - "Column … uses …" for pre-existing untouched columns.
func (l *HasFloatLinter) Lint(existingTables []*statement.CreateTable, changes []*statement.AbstractStatement) (violations []Violation) {
// Loop over all table definitions
for table := range CreateTableStatements(existingTables, changes) {
violations = append(violations, l.checkCreateTable(table)...)
}
pre := PreStateColumns(existingTables)
modified := columnsModifiedInChanges(changes)

// Loop over ALTER TABLE statements that add/modify FLOAT/DOUBLE columns
for _, change := range changes {
alter, ok := change.AsAlterTable()
if !ok {
continue
}
for _, spec := range alter.Specs {
var message string
switch spec.Tp { //nolint:exhaustive
case ast.AlterTableAddColumns:
message = "New column %q in table %q uses floating-point data type"
case ast.AlterTableModifyColumn, ast.AlterTableChangeColumn:
message = "Column %q in table %q modified to use floating-point data type"
default:
for _, ct := range PostState(existingTables, changes) {
tKey := strings.ToLower(ct.TableName)
for _, col := range ct.Columns {
if col.Raw == nil || col.Raw.Tp == nil {
continue
}

// Check all columns in this spec for FLOAT/DOUBLE
for _, col := range spec.NewColumns {
if col.Tp.GetType() == mysql.TypeFloat || col.Tp.GetType() == mysql.TypeDouble {
violations = append(violations, Violation{
Linter: l,
Location: &Location{
Table: change.Table,
Column: &col.Name.Name.O,
},
Message: fmt.Sprintf(message, col.Name.Name.O, change.Table),
Severity: SeverityWarning,
})
}
tp := col.Raw.Tp.GetType()
if tp != mysql.TypeFloat && tp != mysql.TypeDouble {
continue
}
colName := col.Name
colKey := strings.ToLower(colName)
_, preExisted := pre[tKey][colKey]
var message string
switch {
case modified[tKey][colKey]:
// MODIFY / CHANGE COLUMN — the column existed pre-state
// (or is asserted to) and is being retyped.
message = fmt.Sprintf("Column %q in table %q modified to use floating-point data type", colName, ct.TableName)
case !preExisted:
// ADD COLUMN, or a column inside a new CREATE TABLE.
message = fmt.Sprintf("New column %q in table %q uses floating-point data type", colName, ct.TableName)
default:
message = fmt.Sprintf("Column %q in table %q uses %s data type", colName, ct.TableName, col.Raw.Tp.String())
Comment thread
morgo marked this conversation as resolved.
}
}
}
return violations
}

func (l *HasFloatLinter) checkCreateTable(table *statement.CreateTable) (violations []Violation) {
for _, col := range table.Columns {
if col.Raw.Tp.GetType() == mysql.TypeFloat || col.Raw.Tp.GetType() == mysql.TypeDouble {
violations = append(violations, Violation{
Linter: l,
Location: &Location{
Table: table.TableName,
Column: &col.Name,
Table: ct.TableName,
Column: &colName,
},
Message: fmt.Sprintf("Column %q in table %q uses %s data type", col.Name, table.TableName, col.Type),
Message: message,
Severity: SeverityWarning,
})
}
Expand Down
28 changes: 14 additions & 14 deletions pkg/lint/lint_has_float_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,9 @@ func TestHasFloatLinter_AlterTableAddFloatColumn(t *testing.T) {
require.Equal(t, SeverityWarning, violations[0].Severity)
require.Contains(t, violations[0].Message, "temperature")
require.Contains(t, violations[0].Message, "measurements")
// ADD COLUMN should produce the "New column" message form, not "modified".
require.Contains(t, violations[0].Message, "New column")
require.NotContains(t, violations[0].Message, "modified")
require.Equal(t, "measurements", violations[0].Location.Table)
require.Equal(t, "temperature", *violations[0].Location.Column)
}
Expand Down Expand Up @@ -1177,20 +1180,18 @@ func TestHasFloatLinter_UltraComplexScenario(t *testing.T) {
linter := &HasFloatLinter{}
violations := linter.Lint([]*statement.CreateTable{existing1, existing2}, newTableStmts)

// Should detect:
// 1. existing1.value (FLOAT in existing table)
// 2. new_table.score (DOUBLE in new table)
// 3. existing2.pressure (FLOAT via ADD)
// 4. existing2.temperature (DOUBLE via MODIFY)
// 5. existing1.measurement (FLOAT via CHANGE)
require.Len(t, violations, 5)

// Verify we have violations from all sources
var foundExisting1, foundNewTable, foundAdd, foundModify, foundChange bool
// Post-state contains:
// 1. new_table.score — DOUBLE in new CREATE TABLE
// 2. existing2.pressure — FLOAT via ADD
// 3. existing2.temperature — DOUBLE via MODIFY (was INT)
// 4. existing1.measurement — FLOAT via CHANGE (was existing1.value, renamed)
//
// existing1.value is renamed away by CHANGE COLUMN, so it no longer
// appears in the post-state — only the renamed `measurement` does.
require.Len(t, violations, 4)

var foundNewTable, foundAdd, foundModify, foundChange bool
for _, v := range violations {
if v.Location.Table == "existing1" && *v.Location.Column == "value" {
foundExisting1 = true
}
if v.Location.Table == "new_table" && *v.Location.Column == "score" {
foundNewTable = true
}
Expand All @@ -1204,7 +1205,6 @@ func TestHasFloatLinter_UltraComplexScenario(t *testing.T) {
foundChange = true
}
}
require.True(t, foundExisting1, "Should find violation in existing1.value")
require.True(t, foundNewTable, "Should find violation in new_table.score")
require.True(t, foundAdd, "Should find violation in ADD COLUMN")
require.True(t, foundModify, "Should find violation in MODIFY COLUMN")
Expand Down
170 changes: 46 additions & 124 deletions pkg/lint/lint_has_timestamp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ package lint

import (
"fmt"
"strings"

"github.com/block/spirit/pkg/statement"
"github.com/pingcap/tidb/pkg/parser/ast"
"github.com/pingcap/tidb/pkg/parser/mysql"
)

Expand All @@ -26,142 +26,64 @@ func (l *HasTimestampLinter) Description() string {
return "Detects usage of TIMESTAMP data type which overflows on 2038-01-19"
}

// Lint operates on a post-state view of the schema. For each TIMESTAMP column
// in the post-state, the severity is:
//
// - Warning, if the column existed in the pre-state with TIMESTAMP type and
// no incoming change touches it (legacy schema — don't boil the ocean).
// - Error, if the column is newly added in this changeset, or modified to
// TIMESTAMP, or appears in a CREATE TABLE in the changeset.
//
// Columns that were TIMESTAMP in the pre-state but are being dropped or
// converted to a different type in the changeset don't appear in the post-state
// and therefore produce no violation — which is exactly the desired behavior
// for migrations that fix legacy TIMESTAMP columns.
func (l *HasTimestampLinter) Lint(existingTables []*statement.CreateTable, changes []*statement.AbstractStatement) (violations []Violation) {
// Existing tables with TIMESTAMP get Warning — don't boil the ocean on legacy schemas.
for _, ct := range existingTables {
for _, col := range ct.Columns {
if col.Raw.Tp.GetType() == mysql.TypeTimestamp {
violations = append(violations, Violation{
Linter: l,
Location: &Location{
Table: ct.TableName,
Column: &col.Name,
},
Message: fmt.Sprintf("Column %q uses TIMESTAMP which overflows on 2038-01-19. Consider using DATETIME instead.", col.Name),
Severity: SeverityWarning,
})
}
}
}
post := PostState(existingTables, changes)
pre := PreStateColumns(existingTables)
createdInChanges := newTablesInChanges(changes)
addedOrModifiedCols := columnsAddedOrModifiedInChanges(changes)

// CREATE TABLE statements in changes get Error — don't introduce new TIMESTAMP.
for _, change := range changes {
if change.IsCreateTable() {
ct, err := change.ParseCreateTable()
if err != nil {
for _, ct := range post {
tKey := strings.ToLower(ct.TableName)
for _, col := range ct.Columns {
if col.Raw == nil || col.Raw.Tp == nil {
continue
}
for _, col := range ct.Columns {
if col.Raw.Tp.GetType() == mysql.TypeTimestamp {
violations = append(violations, Violation{
Linter: l,
Location: &Location{
Table: ct.TableName,
Column: &col.Name,
},
Message: fmt.Sprintf("Column %q uses TIMESTAMP which overflows on 2038-01-19. Consider using DATETIME instead.", col.Name),
Severity: SeverityError,
})
}
if col.Raw.Tp.GetType() != mysql.TypeTimestamp {
continue
}
colName := col.Name
severity := l.severityFor(tKey, colName, pre, createdInChanges, addedOrModifiedCols)
violations = append(violations, Violation{
Linter: l,
Location: &Location{
Table: ct.TableName,
Column: &colName,
},
Message: fmt.Sprintf("Column %q uses TIMESTAMP which overflows on 2038-01-19. Consider using DATETIME instead.", colName),
Severity: severity,
})
}
}

// Check ALTER TABLE statements
violations = append(violations, l.checkAlterStatements(existingTables, changes)...)

return violations
}

// checkAlterStatements checks ALTER TABLE statements for TIMESTAMP usage.
// Adding or modifying a column to TIMESTAMP is an Error. Modifying a table that
// already has TIMESTAMP columns (but not adding new ones) is a Warning — unless
// the ALTER is actively removing/converting those TIMESTAMP columns.
func (l *HasTimestampLinter) checkAlterStatements(existingTables []*statement.CreateTable, changes []*statement.AbstractStatement) (violations []Violation) {
// Build a map of existing tables for quick lookup
existingTableMap := make(map[string]*statement.CreateTable)
for _, table := range existingTables {
existingTableMap[table.GetTableName()] = table
func (l *HasTimestampLinter) severityFor(tableKey, colName string, pre map[string]map[string]*statement.Column, createdTables map[string]bool, addedOrModified map[string]map[string]bool) Severity {
colKey := strings.ToLower(colName)
if createdTables[tableKey] {
return SeverityError
}

for _, change := range changes {
alter, ok := change.AsAlterTable()
if !ok {
continue
if tableMods, ok := addedOrModified[tableKey]; ok {
if tableMods[colKey] {
return SeverityError
}

// Track whether the ALTER is adding/modifying columns to TIMESTAMP (Error),
// and collect the set of columns being dropped or converted away from TIMESTAMP.
addingTimestamp := false
columnsBeingFixed := make(map[string]bool)

for _, spec := range alter.Specs {
switch spec.Tp { //nolint:exhaustive
case ast.AlterTableAddColumns:
for _, col := range spec.NewColumns {
if col.Tp.GetType() == mysql.TypeTimestamp {
addingTimestamp = true
violations = append(violations, Violation{
Linter: l,
Location: &Location{
Table: change.Table,
Column: &col.Name.Name.O,
},
Message: fmt.Sprintf("Column %q uses TIMESTAMP which overflows on 2038-01-19. Consider using DATETIME instead.", col.Name.Name.O),
Severity: SeverityError,
})
}
}
case ast.AlterTableModifyColumn, ast.AlterTableChangeColumn:
for _, col := range spec.NewColumns {
if col.Tp.GetType() == mysql.TypeTimestamp {
addingTimestamp = true
violations = append(violations, Violation{
Linter: l,
Location: &Location{
Table: change.Table,
Column: &col.Name.Name.O,
},
Message: fmt.Sprintf("Column %q uses TIMESTAMP which overflows on 2038-01-19. Consider using DATETIME instead.", col.Name.Name.O),
Severity: SeverityError,
})
} else {
// Column is being changed to a non-TIMESTAMP type — it's being fixed.
// For CHANGE COLUMN, OldColumnName is the original name.
if spec.OldColumnName != nil {
columnsBeingFixed[spec.OldColumnName.Name.O] = true
}
columnsBeingFixed[col.Name.Name.O] = true
}
}
case ast.AlterTableDropColumn:
if spec.OldColumnName != nil {
columnsBeingFixed[spec.OldColumnName.Name.O] = true
}
}
}

// If the ALTER is not itself introducing TIMESTAMP columns, check whether
// the existing table already has TIMESTAMP columns (Warning).
// Exclude columns that are being dropped or converted in this ALTER.
if !addingTimestamp {
if existing, ok := existingTableMap[change.Table]; ok {
for _, col := range existing.Columns {
if col.Raw.Tp.GetType() == mysql.TypeTimestamp && !columnsBeingFixed[col.Name] {
violations = append(violations, Violation{
Linter: l,
Location: &Location{
Table: change.Table,
Column: &col.Name,
},
Message: fmt.Sprintf("Column %q uses TIMESTAMP which overflows on 2038-01-19. Consider using DATETIME instead.", col.Name),
Severity: SeverityWarning,
})
}
}
}
}
if preTbl, ok := pre[tableKey]; ok {
if preCol, ok := preTbl[colKey]; ok && preCol.Raw != nil && preCol.Raw.Tp != nil && preCol.Raw.Tp.GetType() == mysql.TypeTimestamp {
return SeverityWarning
}
}

return violations
return SeverityError
}
Loading
Loading