fix(#828): backtick-quote PK column names in chunker and checksum SQL#829
Merged
Merged
Conversation
…m SQL The chunker_composite prefetch query, the chunker_optimistic prefetch query, the single-checker inspectDifferences query, and the table-info min/max query all built SQL with bare PK column names. When a primary key column is a MySQL reserved word (e.g. `key`, `value`), the migration fails immediately with Error 1064. Quote PK columns at every SQL composition site, and add regression tests exercising both chunkers, the migrate path, and the move path with a reserved-word composite PK. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes migrations/moves/checksums failing with MySQL Error 1064 when primary key columns are reserved words (e.g. key, value) by consistently backtick-quoting PK column identifiers in generated SQL.
Changes:
- Backtick-quote PK column identifiers in chunker prefetch queries (composite + optimistic) and TableInfo min/max query.
- Backtick-quote PK column list passed into checksum row inspection query template.
- Add regression tests covering reserved-word PKs across chunkers, migration E2E, and move E2E.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/table/tableinfo.go | Quotes PK column in min/max SQL used during table info setup. |
| pkg/table/chunker_optimistic.go | Quotes single-column auto-inc PK in optimistic chunker prefetch SQL. |
| pkg/table/chunker_composite.go | Uses QuoteColumns for SELECT/ORDER BY lists in composite chunker prefetch SQL. |
| pkg/checksum/single.go | Uses table.QuoteColumns for PK list in queryTemplate used by row inspection. |
| pkg/table/chunker_optimistic_test.go | Adds regression test for optimistic prefetch path with reserved-word PK column. |
| pkg/table/chunker_composite_test.go | Adds regression test walking composite chunker on reserved-word composite PK. |
| pkg/migration/e2e_test.go | Adds E2E migration regression test for reserved-word composite PK. |
| pkg/move/move_test.go | Adds E2E move regression test for reserved-word composite PK. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot feedback on block#829 — replace hand-rolled backtick concatenation with the existing QuoteColumns helper for consistency with the rest of the codebase. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Kiran01bm
approved these changes
May 8, 2026
aparajon
approved these changes
May 8, 2026
Collaborator
|
these needs fixing as well ?
|
Address Kiran's review comment on block#829 — t.keyName was passed into FORCE INDEX(...) unquoted, so a secondary index whose name is a MySQL reserved word (e.g. `key`) would also break the prefetch query. Extend the regression test to cover this case via SetKey. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Good catch — yes, the index name passed into |
… move QuotedTableName is already backtick-wrapped at construction, so these pass without a code change — they guard against any future regression where a SQL builder uses the unquoted TableName. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Fixes #828.
Summary
key,value) failed immediately with Error 1064 because several SQL builders interpolated bare PK column names.pkg/table/chunker_composite.go::Next()— composite prefetch query (the user-reported failure path).pkg/table/chunker_optimistic.go::nextChunkByPrefetching— single-key prefetch query.pkg/table/tableinfo.go::setMinMax— min/max query that runs duringSetInfo.pkg/checksum/single.go::inspectDifferences— PK list passed intoqueryTemplate.table.QuoteColumnshelper.Note on origin
This is not a regression from #701/#702. The unquoted
strings.Joinin the composite chunker has been there since36877bb(2023-08-02), when composite-key chunking was first introduced. PR #702 doesn't touch the prefetch query. The bug surfaced now only because no existing test covered a reserved-word PK column.Test plan
pkg/tableregression test: composite chunker walks a table with PK(osm_id, \key`)` to completion.pkg/tableregression test: optimistic chunker prefetch path with a single-column auto-inc PK named\key``.pkg/migrationregression test: end-to-end ALTER through the copy path (the user's reported scenario).pkg/moveregression test: end-to-end move with a reserved-word composite PK.pkg/table,pkg/checksum,pkg/copiertest suites still pass locally.🤖 Generated with Claude Code