Type-aware encoder for streaming binary rows#1132
Conversation
![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 introduces EncodeBinaryFieldValue, a robust and type-aware replacement for FormatBinaryValue in the MySQL binary protocol implementation. The new implementation correctly handles fixed-width integers with range-checking and implements packed temporal formats for DATE, DATETIME, and TIME types, ensuring compatibility with narrow-width columns that were previously incorrectly encoded as 8-byte values. Additionally, the streaming row writer in server/resp.go was optimized to reuse the NULL bitmap buffer, and extensive unit tests were added to verify encoding accuracy across various data types and edge cases. I have no feedback to provide as no review comments were submitted.
|
For the failing check: diff --git a/mysql/resultset_helper_test.go b/mysql/resultset_helper_test.go
index bc91a63..fa86d08 100644
--- a/mysql/resultset_helper_test.go
+++ b/mysql/resultset_helper_test.go
@@ -544,4 +544,3 @@ func TestEncodeBinaryFieldValueTypedNilSlice(t *testing.T) {
require.Equal(t, "after", string(row[1].AsString()))
})
}
- |
Currently, the streaming binary path produces malformed wire bytes for several column types: * narrow integers (`TINY`, `SHORT`, `INT24`, `LONG`) get 8 bytes regardless of declared width * `FLOAT` is encoded as `Float64bits` truncated to 4 bytes, which is wrong * variable-length types other than `VAR_STRING` get unframed payloads * temporals are sent as parser-formatted ASCII instead of the packed binary form This PR adds a new `EncodeBinaryFieldValue` function and routes `writeStreamBinaryRows` through it. This new encoder honors the declared column type for width, framing, and packing. Out-of-range and malformed inputs are now rejected loudly instead of silently corrupting the wire: * per-type integer widths with range checks * `Float32bits` for `FLOAT`, with overflow rejection * length-encoded variable-width types * packed temporals, re-packed from formatted strings as needed * `MYSQL_TYPE_YEAR` forced unsigned * `bool` only for `MYSQL_TYPE_TINY` * empty strings, trailing whitespace and dots, microseconds over 6 digits, and `TIME` hours over 838 are rejected `BuildSimpleBinaryResultset` is untouched, but consolidating it onto the new encoder is coming in a follow-up PR.
ramnes
force-pushed
the
ramnes/type-aware
branch
from
d541877 to
222645a
Compare
ramnes
force-pushed
the
ramnes/type-aware
branch
from
0e2180e to
213ad7b
Compare
|
@dveeden Thanks for the review, both fixed! |
| switch v := value.(type) { | ||
| case []byte: | ||
| if v == nil { | ||
| // Typed-nil []byte → NULL. The top-level nil check doesn't |
There was a problem hiding this comment.
[P1] []byte(nil) is treated as NULL here, but RowData.ParseBinary can produce []byte(nil) for empty length-encoded strings (len=0) via append(dst[:0], v...) when v is empty. In a proxy flow (parse → FieldValue.Value() → encode), empty strings can therefore be re-encoded as NULL (NULL-bitmap set, no payload). Consider preserving empty-string vs NULL by ensuring the parser returns a non-nil zero-length slice for empty strings, or by encoding []byte(nil) as length=0 for string-like column types and reserving NULL for nil interface / driver.Valuer→nil / MYSQL_TYPE_NULL.
(AI review)
|
Left an inline note on a potential empty-string vs NULL re-encoding edge case. I’ll do a full human review of the PR soon. |
| return []byte{0}, nil | ||
| } | ||
| year := t.Year() | ||
| if year < 0 || year > 9999 { |
There was a problem hiding this comment.
Can you explain how this range limit is chosen? Maybe it's from the 4 digits of YEAR type?
| } | ||
|
|
||
| // toBinaryTime encodes a time.Time as a length-prefixed packed binary TIME. | ||
| // For negative or >23h values, pass a string instead. |
There was a problem hiding this comment.
pass a string
Do you mean parseTimeString?
| if hasDot && frac == "" { | ||
| return nil, errors.Errorf("invalid TIME %q: trailing dot without fractional digits", s) | ||
| } | ||
| parts := strings.Split(hms, ":") |
There was a problem hiding this comment.
Seems can reuse parseHMSFraction
| // MySQL fractional seconds top out at 6 digits. | ||
| return 0, errors.Errorf("invalid microseconds %q: more than 6 digits", s) | ||
| } | ||
| for len(s) < 6 { |
There was a problem hiding this comment.
nit: maybe * power of 10 after ParseUint
3 participants
Currently, the streaming binary path produces malformed wire bytes for several column types:
TINY,SHORT,INT24,LONG) get 8 bytes regardless of declared widthFLOATis encoded asFloat64bitstruncated to 4 bytes, which is wrongVAR_STRINGget unframed payloadsThis PR adds a new
EncodeBinaryFieldValuefunction and routeswriteStreamBinaryRowsthrough it. This new encoder honors the declared column type for width, framing, and packing.Out-of-range and malformed inputs are now rejected loudly instead of silently corrupting the wire:
Float32bitsforFLOAT, with overflow rejectionMYSQL_TYPE_YEARforced unsignedboolonly forMYSQL_TYPE_TINYTIMEhours over 838 are rejectedBuildSimpleBinaryResultsetis untouched, but consolidating it onto the new encoder is coming in a follow-up PR.