Conversation
There was a problem hiding this comment.
Pull request overview
Fixes incorrect BIT unique-key range comparisons by normalizing BIT values to unsigned integers and ensuring prepared range queries compare them in numeric context, preventing infinite chunk loops / missing rows during rowcopy.
Changes:
- Add
BitColumnTypedetection (viainformation_schema.columns) and convertBITvalues touint64inconvertArg. - Render
BITprepared placeholders ascast(? as unsigned)in range/other prepared comparisons. - Normalize min/max/iteration range values after scanning so range queries consistently use converted args; add unit + local integration coverage.
Show a summary per file
| File | Description |
|---|---|
| localtests/bit-unique-key/create.sql | Adds a local integration scenario with BIT in a composite PK. |
| go/sql/types_test.go | Adds a unit test for BIT argument conversion to uint64. |
| go/sql/types.go | Introduces BitColumnType, converts BIT args to uint64, and adds ColumnValues.NormalizeValues. |
| go/sql/builder.go | Ensures prepared comparisons for BIT use numeric casting (cast(? as unsigned)). |
| go/logic/inspect.go | Detects BIT columns and marks them as BitColumnType. |
| go/logic/applier.go | Normalizes scanned min/max/iteration-range values so range queries use converted args. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 3
| for i, col := range columns.Columns() { | ||
| this.abstractValues[i] = col.convertArg(this.abstractValues[i]) | ||
| } |
There was a problem hiding this comment.
NormalizeValues assumes len(this.abstractValues) >= columns.Len() and will panic with an index-out-of-range if called with a mismatched ColumnList (or a ColumnValues created with the wrong length). Consider adding a length check (and returning an error) or iterating up to min(len(values), columns.Len()) with a clear failure path when they differ, so this can't crash the process unexpectedly.
| for i, col := range columns.Columns() { | |
| this.abstractValues[i] = col.convertArg(this.abstractValues[i]) | |
| } | |
| cols := columns.Columns() | |
| limit := len(cols) | |
| if len(this.abstractValues) < limit { | |
| limit = len(this.abstractValues) | |
| } | |
| for i := 0; i < limit; i++ { | |
| this.abstractValues[i] = cols[i].convertArg(this.abstractValues[i]) | |
| } |
| this.migrationContext.Log.Infof("Migration min values: [%s]", this.migrationContext.MigrationRangeMinValues) | ||
| if this.migrationContext.MigrationRangeMinValues != nil { | ||
| this.migrationContext.MigrationRangeMinValues.NormalizeValues(uniqueKey.Columns) | ||
| } |
There was a problem hiding this comment.
BIT normalization is applied for min/max range reads, but the resume-from-checkpoint path populates MigrationIterationRangeMinValues/MaxValues from _ghk via ReadLastCheckpoint() and those values are never normalized. Since the checkpoint table uses the original column types (including BIT), scanned values will still be []uint8, which can reintroduce the incorrect range-comparison behavior (infinite loop / missing rows) after resume. Consider normalizing the checkpoint-derived IterationRangeMin/Max (e.g., inside ReadLastCheckpoint() before returning, or immediately after assigning them during resume).
| } else if column.Type == BitColumnType { | ||
| token = "cast(? as unsigned)" | ||
| } else { |
There was a problem hiding this comment.
The new BitColumnType placeholder rendering (cast(? as unsigned)) in buildColumnsPreparedValues isn't covered by existing unit tests in builder_test.go. Adding a small test that builds a range/ checkpoint query with a BIT column and asserts the generated SQL contains the cast would help prevent regressions in this comparison fix.
Related issue: #1480
BITcolumn values aren't compared correctly because gh-ost stores them as binary strings. Instead they should be compared as 64-bit unsigned integers (https://dev.mysql.com/doc/refman/8.4/en/bit-type.html). The fix is to convert them to uint64 inconvertArgand make sureconvertArgis called for all range queries.script/cibuildreturns with no formatting errors, build errors or unit test errors.