Minor changes from self review

Signed-off-by: Matt Lord <[email protected]>

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 {

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.

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)

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++

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'")

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 {

proto/binlogdata.proto

@@ -336,9 +336,9 @@ message RowChange {
   // DataColumns is a bitmap of all columns: bit is set if column is
   // present in the after image.
   Bitmap data_columns = 3;
-  // 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