From 546db2874ffc81de55184a2b88e85a1a85625554 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Tue, 10 Dec 2024 20:03:59 -0500 Subject: [PATCH] Minor changes from self review Signed-off-by: Matt Lord --- go/mysql/binlog/binlog_json.go | 14 ++++++++------ go/mysql/binlog_event.go | 7 ++++--- go/mysql/binlog_event_mysql56_test.go | 1 + go/mysql/binlog_event_rbr.go | 2 +- go/test/endtoend/vreplication/vreplication_test.go | 2 ++ go/vt/proto/binlogdata/binlogdata.pb.go | 6 +++--- .../tabletmanager/vreplication/replicator_plan.go | 8 +++++--- 7 files changed, 24 insertions(+), 16 deletions(-) diff --git a/go/mysql/binlog/binlog_json.go b/go/mysql/binlog/binlog_json.go index 29c37d1eb78..e3ca52096f2 100644 --- a/go/mysql/binlog/binlog_json.go +++ b/go/mysql/binlog/binlog_json.go @@ -71,14 +71,14 @@ func ParseBinaryJSON(data []byte) (*json.Value, error) { return node, nil } -// ParseBinaryJSONDiff provides the parsing function from the MySQL JSON -// diff representation to an SQL expression. +// ParseBinaryJSONDiff provides the parsing function from the binary MySQL +// JSON diff representation to an SQL expression. func ParseBinaryJSONDiff(data []byte) (sqltypes.Value, error) { diff := bytes.Buffer{} // Reasonable estimate of the space we'll need to build the SQL // expression in order to try and avoid reallocations w/o // overallocating too much. - diff.Grow(int(float32(len(data)) * 1.5)) + diff.Grow(int(float32(len(data)) * 1.25)) pos := 0 outer := false innerStr := "" @@ -98,12 +98,13 @@ func ParseBinaryJSONDiff(data []byte) (sqltypes.Value, error) { case jsonDiffOpRemove: diff.WriteString("JSON_REMOVE(") default: - // Can be a literal JSON null. + // Can be a JSON null. js, err := ParseBinaryJSON(data) if err == nil && js.Type() == json.TypeNull { return sqltypes.MakeTrusted(sqltypes.Expression, js.MarshalTo(nil)), nil } - return sqltypes.Value{}, fmt.Errorf("invalid JSON diff operation: %d", opType) + return sqltypes.Value{}, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, + "invalid JSON diff operation: %d", opType) } if outer { diff.WriteString(innerStr) @@ -129,7 +130,8 @@ func ParseBinaryJSONDiff(data []byte) (sqltypes.Value, error) { pos = readTo value, err := ParseBinaryJSON(data[pos : pos+valueLen]) if err != nil { - return sqltypes.Value{}, fmt.Errorf("cannot read JSON diff value for path %s: %w", path, err) + return sqltypes.Value{}, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, + "cannot read JSON diff value for path %s: %v", path, err) } pos += valueLen if value.Type() == json.TypeString { diff --git a/go/mysql/binlog_event.go b/go/mysql/binlog_event.go index 76660f4ee90..72c228b594e 100644 --- a/go/mysql/binlog_event.go +++ b/go/mysql/binlog_event.go @@ -270,9 +270,10 @@ type Row struct { // It is only set for UPDATE and DELETE events. Identify []byte - // If this row represents a PartialUpdateRow event there will be a - // shared-image that sits between the before and after images that - // defines how the JSON column values are represented. + // If this row was from a PartialUpdateRows event and it contains + // 1 or more JSON columns with partial values, then this will be + // set as a bitmap of which JSON columns in the AFTER image have + // partial values. JSONPartialValues Bitmap // Data is the raw data. diff --git a/go/mysql/binlog_event_mysql56_test.go b/go/mysql/binlog_event_mysql56_test.go index a138f28e16e..77ce2c11d60 100644 --- a/go/mysql/binlog_event_mysql56_test.go +++ b/go/mysql/binlog_event_mysql56_test.go @@ -594,6 +594,7 @@ func TestMySQL56PartialUpdateRowsEvent(t *testing.T) { assert.Equal(t, tc.numRows, len(ev.Rows)) require.NoError(t, err) + for i := range ev.Rows { vals, err := ev.StringValuesForTests(tm, i) require.NoError(t, err) diff --git a/go/mysql/binlog_event_rbr.go b/go/mysql/binlog_event_rbr.go index 9612780b3e8..f4f005b5dcd 100644 --- a/go/mysql/binlog_event_rbr.go +++ b/go/mysql/binlog_event_rbr.go @@ -370,7 +370,7 @@ func (ev binlogEvent) Rows(f BinlogFormat, tm *TableMap) (Rows, error) { if ev.Type() == ePartialUpdateRowsEvent { // The first byte indicates whether or not any JSON values are partial. - // If it's not 1 then there's nothing else to do for the row as any + // If it's not 1 then there's nothing special to do for the row as any // columns use the full value. partialJSON := uint8(data[pos]) pos++ diff --git a/go/test/endtoend/vreplication/vreplication_test.go b/go/test/endtoend/vreplication/vreplication_test.go index d1c7f5aa8ea..870b5d0d348 100644 --- a/go/test/endtoend/vreplication/vreplication_test.go +++ b/go/test/endtoend/vreplication/vreplication_test.go @@ -724,6 +724,8 @@ func shardCustomer(t *testing.T, testReverse bool, cells []*Cell, sourceCellOrAl // TODO: file a MySQL bug for this. The following query results in the quoted string literal "null" // stored in the j3 column. But with and without the literal quotes, the value in the PARTIAL_JSON // diff is the unquoted literal null which is a JSON null type. This leads to a vdiff mismatch. + // I'm not sure if the bug is that it allows the quoted value "null" to be inserted or that it + // doesn't reflect this in the partial diff value, but the combination of the two is certainly a bug. //execVtgateQuery(t, vtgateConn, sourceKs, "update json_tbl set j1 = null, j2 = 'null', j3 = '\"null\"'") //execVtgateQuery(t, vtgateConn, sourceKs, "insert into json_tbl(id, j1, j2, j3) values (7, null, 'null', '\"null\"')") execVtgateQuery(t, vtgateConn, sourceKs, "update json_tbl set j1 = null, j2 = 'null', j3 = 'null'") diff --git a/go/vt/proto/binlogdata/binlogdata.pb.go b/go/vt/proto/binlogdata/binlogdata.pb.go index 0cc9d5b17cf..e3523b6b384 100644 --- a/go/vt/proto/binlogdata/binlogdata.pb.go +++ b/go/vt/proto/binlogdata/binlogdata.pb.go @@ -1336,9 +1336,9 @@ type RowChange struct { // DataColumns is a bitmap of all columns: bit is set if column is // present in the after image. DataColumns *RowChange_Bitmap `protobuf:"bytes,3,opt,name=data_columns,json=dataColumns,proto3" json:"data_columns,omitempty"` - // JsonPartialValues is a bitmap of any JSON columns where the bit is - // set if the value in the after image is a partial JSON value that - // is represented as an expression of + // JsonPartialValues is a bitmap of any JSON columns, where the bit + // is set if the value in the AFTER image is a partial JSON value + // that is represented as an expression of // JSON_[INSERT|REPLACE|REMOVE](%s, '$.path', value) which then is // used to add/update/remove a path in the JSON document. When the // value is used the fmt directive must be replaced by the actual diff --git a/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go b/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go index 7a51d5f5aea..206a1d9f037 100644 --- a/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go +++ b/go/vt/vttablet/tabletmanager/vreplication/replicator_plan.go @@ -394,12 +394,14 @@ func (tp *TablePlan) applyChange(rowChange *binlogdatapb.RowChange, executor fun !slices.Equal(vals[i].Raw(), sqltypes.NullBytes): // An SQL expression that can be converted to a JSON value such // as JSON_INSERT(). - // This occurs e.g. when using partial JSON values as a result of + // This occurs when using partial JSON values as a result of // mysqld using binlog-row-value-options=PARTIAL_JSON. if len(vals[i].Raw()) == 0 { // When using BOTH binlog_row_image=NOBLOB AND - // binlog_row_value_options=PARTIAL_JSON then the JSON column - // has the data bit set and the diff is empty. + // binlog_row_value_options=PARTIAL_JSON then the JSON + // column has the data bit set and the diff is empty. So + // we have to account for this by unsetting the data bit + // so that the current JSON value is not overwritten. setBit(rowChange.DataColumns.Cols, i, false) newVal = ptr.Of(sqltypes.MakeTrusted(querypb.Type_EXPRESSION, nil)) } else {