syncer(dm): centralize row image layout mapping#12750
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. |
ti-chi-bot
Bot
added
size/XL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes the mapping of binlog row images to source table columns by introducing the RowImageLayout struct in pkg/sqlmodel/row_image_layout.go, replacing duplicated helper functions and the rowValueMapper struct. The review feedback highlights several critical safety improvements to prevent potential panics. Specifically, it points out potential out-of-bounds slice indexing in NewRowImageLayoutFromColumns, FullValues, and valueOffset when handling sliced columns with original offsets, as well as a missing nil-pointer check in tableInfoForVisibleColumnCount.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func NewRowImageLayoutFromColumns(sourceColumns, targetColumns []*model.ColumnInfo) RowImageLayout { | ||
| visibleColumns := VisibleColumns(sourceColumns) | ||
| visibleOffsetByColumnOffset := make([]int, len(sourceColumns)) | ||
| for i := range visibleOffsetByColumnOffset { | ||
| visibleOffsetByColumnOffset[i] = -1 | ||
| } | ||
| for i, column := range visibleColumns { | ||
| visibleOffsetByColumnOffset[column.Offset] = i | ||
| } | ||
|
|
||
| layout := RowImageLayout{ | ||
| columns: sourceColumns, | ||
| visibleColumns: visibleColumns, | ||
| visibleOffsetByColumnOffset: visibleOffsetByColumnOffset, | ||
| } | ||
| if targetColumns != nil { | ||
| layout.writableColumns = writableSourceColumns(visibleColumns, targetColumns) | ||
| } | ||
| return layout | ||
| } |
There was a problem hiding this comment.
In NewRowImageLayoutFromColumns, visibleOffsetByColumnOffset is sized to len(sourceColumns). However, if sourceColumns is a sliced columns list (e.g., clone.Columns[:sourceColumnCount]), len(sourceColumns) will be smaller than the original column count, but the individual column.Offset values will still retain their original values. This can cause visibleOffsetByColumnOffset[column.Offset] = i to panic with an out-of-bounds index. Sizing the slice based on the maximum offset found in sourceColumns avoids this panic.
func NewRowImageLayoutFromColumns(sourceColumns, targetColumns []*model.ColumnInfo) RowImageLayout {\n\tvisibleColumns := VisibleColumns(sourceColumns)\n\tmaxOffset := -1\n\tfor _, column := range sourceColumns {\n\t\tif column.Offset > maxOffset {\n\t\t maxOffset = column.Offset\n\t\t}\n\t}\n\tvisibleOffsetByColumnOffset := make([]int, maxOffset+1)\n\tfor i := range visibleOffsetByColumnOffset {\n\t\tvisibleOffsetByColumnOffset[i] = -1\n\t}\n\tfor i, column := range visibleColumns {\n\t\tif column.Offset >= 0 && column.Offset <= maxOffset {\n\t\t visibleOffsetByColumnOffset[column.Offset] = i\n\t\t}\n\t}\n\n\tlayout := RowImageLayout{\n\t\tcolumns: sourceColumns,\n\t\tvisibleColumns: visibleColumns,\n\t\tvisibleOffsetByColumnOffset: visibleOffsetByColumnOffset,\n\t}\n\tif targetColumns != nil {\n\t\tlayout.writableColumns = writableSourceColumns(visibleColumns, targetColumns)\n\t}\n\treturn layout\n}| func tableInfoForVisibleColumnCount(tableInfo *model.TableInfo, columnCount int) (*model.TableInfo, bool) { | ||
| visibleCount := 0 | ||
| stripColumnCount := len(tableInfo.Columns) | ||
| for i, col := range tableInfo.Columns { | ||
| if col.Hidden { | ||
| continue | ||
| } | ||
| visibleCount++ | ||
| if visibleCount > columnCount { | ||
| stripColumnCount = i | ||
| break | ||
| } | ||
| } | ||
| if visibleCount < columnCount { | ||
| layout := sqlmodel.NewRowImageLayoutFromColumns(tableInfo.Columns, nil) | ||
| sourceColumnCount, ok := layout.SourceColumnCountForVisibleColumnCount(columnCount) | ||
| if !ok { | ||
| return nil, false | ||
| } | ||
| if visibleCount == columnCount { | ||
| if sourceColumnCount == len(tableInfo.Columns) { | ||
| return tableInfo, true | ||
| } | ||
|
|
||
| clone := tableInfo.Clone() | ||
| clone.Columns = clone.Columns[:stripColumnCount] | ||
| clone.Columns = clone.Columns[:sourceColumnCount] | ||
| return clone, true | ||
| } |
There was a problem hiding this comment.
The function tableInfoForVisibleColumnCount does not check if the input tableInfo is nil. If a nil pointer is passed, calling tableInfo.Columns will cause a nil pointer dereference panic. Adding a defensive nil check at the beginning of the function will make it more robust.
func tableInfoForVisibleColumnCount(tableInfo *model.TableInfo, columnCount int) (*model.TableInfo, bool) {\n\tif tableInfo == nil {\n\t return nil, false\n\t}\n\tlayout := sqlmodel.NewRowImageLayoutFromColumns(tableInfo.Columns, nil)\n\tsourceColumnCount, ok := layout.SourceColumnCountForVisibleColumnCount(columnCount)\n\tif !ok {\n\t return nil, false\n\t}\n\tif sourceColumnCount == len(tableInfo.Columns) {\n\t return tableInfo, true\n\t}\n\n\tclone := tableInfo.Clone()\n\tclone.Columns = clone.Columns[:sourceColumnCount]\n\treturn clone, true\n}| func (l RowImageLayout) FullValues(row []any) ([]any, bool) { | ||
| if len(row) == len(l.columns) { | ||
| return row, true | ||
| } | ||
| if len(row) != len(l.visibleColumns) { | ||
| return nil, false | ||
| } | ||
|
|
||
| fullValues := make([]any, len(l.columns)) | ||
| for i, col := range l.visibleColumns { | ||
| fullValues[col.Offset] = row[i] | ||
| } | ||
| return fullValues, true | ||
| } |
There was a problem hiding this comment.
In FullValues, fullValues is sized to len(l.columns). If col.Offset is greater than or equal to len(l.columns) (which can happen if the columns slice is sliced or has non-contiguous offsets), assigning fullValues[col.Offset] = row[i] will panic with an out-of-bounds index. Adding a defensive bounds check prevents this panic.
func (l RowImageLayout) FullValues(row []any) ([]any, bool) {\n\tif len(row) == len(l.columns) {\n\t return row, true\n\t}\n\tif len(row) != len(l.visibleColumns) {\n\t return nil, false\n\t}\n\n\tfullValues := make([]any, len(l.columns))\n\tfor i, col := range l.visibleColumns {\n\t\tif col.Offset >= 0 && col.Offset < len(fullValues) {\n\t\t fullValues[col.Offset] = row[i]\n\t\t}\n\t}\n\treturn fullValues, true\n}| func (l RowImageLayout) valueOffset(columnOffset int, values []any) int { | ||
| if l.isFullValues(values) { | ||
| return columnOffset | ||
| } | ||
| return l.visibleOffsetByColumnOffset[columnOffset] | ||
| } |
There was a problem hiding this comment.
In valueOffset, accessing l.visibleOffsetByColumnOffset[columnOffset] directly can panic if columnOffset is out of bounds (e.g., if it is negative or greater than or equal to the length of the slice). Adding a defensive bounds check makes the method safer.
func (l RowImageLayout) valueOffset(columnOffset int, values []any) int {\n\tif l.isFullValues(values) {\n\t return columnOffset\n\t}\n\tif columnOffset < 0 || columnOffset >= len(l.visibleOffsetByColumnOffset) {\n\t return -1\n\t}\n\treturn l.visibleOffsetByColumnOffset[columnOffset]\n}1 participant
What problem does this PR solve?
This is a follow-up draft for centralizing row image layout handling after the hidden/generated column fixes. The current code has similar source-column/visible-row/writable-column mapping logic in
WhereHandle,RowChange, expression filters, and validator code, which makes hidden-column layout handling easy to drift.Issue Number: ref #12747
What is changed and how it works?
This PR adds
sqlmodel.RowImageLayoutas the stage-1 shared representation for row image layout metadata:WhereHandleandRowChange.dmlRowMapping()now build their DML/WHERE value mapping from this layout. DM expression filter and validator paths also reuse the same layout helpers instead of maintaining local hidden-column filtering/mapping logic.Check List
Tests
Verified locally:
Questions
Will it cause performance regression or break compatibility?
No expected compatibility impact. This is intended as an internal refactor. Performance impact should be neutral: the new layout replaces existing mapper/helper construction and removes duplicated mapping logic.
Do you need to update user documentation, design documentation or monitoring documentation?
No. This is an internal code structure change.
Release note