Skip to content

Commit

Permalink
Switch vizierapi over to use bytes instead of string for StringColumn
Browse files Browse the repository at this point in the history
Summary:
Fixes #552: GRPC C clients reject parsing string values from the serialized format
if the string is not valid UTF-8. The string and bytes pb types both serialize to bytes
over the wire, so we didn't detect this issue until someone requested http body data that contained
a bytes stream.

Fortunately the fix isn't difficult because string and bytes share the same wire format. Old clients
will remain backwards compatible with the current changes.

Also internally we already treat this data as bytes so conceptually not a large switch.

Test Plan:
1. Old client + new vizier still works as expected when all strings are utf-8 compatible.
2. New client + old vizier now avoids the issue highlighted in #552 when Pixie sends bytes data
3. New client + new vizier also avoids the issue and works fine.

Reviewers: jamesbartlett, zasgar, michelle, vihang, nlanam

Reviewed By: vihang, nlanam

Subscribers: nlanam

Signed-off-by: Phillip Kuznetsov <[email protected]>

Differential Revision: https://phab.corp.pixielabs.ai/D12069

GitOrigin-RevId: 73b4698d764af8a2f3834ae635a195cfe9a18cb6
  • Loading branch information
Phillip Kuznetsov authored and copybaranaut committed Aug 29, 2022
1 parent 3272420 commit 49e6790
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 16 deletions.
26 changes: 13 additions & 13 deletions proto/vizierpb/vizierapi.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion proto/vizierpb/vizierapi.proto
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ message Time64NSColumn {

// String data column.
message StringColumn {
repeated string data = 1;
repeated bytes data = 1;
}

// A single column of data.
Expand Down
2 changes: 1 addition & 1 deletion results.go
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ func extractDataFromCol(colData []*vizierpb.Column, rowIdx, colIdx int64, row []
if !ok {
return errdefs.ErrInternalMismatchedType
}
dCasted.ScanString(colTyped.StringData.Data[rowIdx])
dCasted.ScanString(string(colTyped.StringData.Data[rowIdx]))
case *vizierpb.Column_Uint128Data:
dCasted, ok := row[colIdx].(*types.UInt128Value)
if !ok {
Expand Down
6 changes: 5 additions & 1 deletion results_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,14 @@ func makeInt64Column(data []int64) *vizierpb.Column {
}

func makeStringColumn(data []string) *vizierpb.Column {
b := make([][]byte, len(data))
for i, d := range data {
b[i] = []byte(d)
}
return &vizierpb.Column{
ColData: &vizierpb.Column_StringData{
StringData: &vizierpb.StringColumn{
Data: data,
Data: b,
},
},
}
Expand Down

0 comments on commit 49e6790

Please sign in to comment.