sql/types: types.EnumMetadata implements encoding.TextMarshaler interface#72640
Conversation
|
This change is |
395ae5c to
309f765
Compare
|
pkg/sql/types/types.go, line 231 at r1 (raw file):
I'm totally open to remove this kind of logic to insert separator, we would just have an extra comma at the end... |
ajwerner
left a comment
There was a problem hiding this comment.
The more I think about it, the less I think I care about actually populating these. The biggest reason is that the type metadata in the cases we're dealing with should not be populated. The point of the types.T having the embedded internal type and then the type metadata is to support in-memory operations and to make sure we never, ever serialize the user-defined metadata to disk. The reason for all of this is that the user-defined metadata can change and needs to be "hydrated" into the type descriptor. This is a topic we can unpack at some future point.
I think what I'd prefer is that we just skip the user defined type metadata altogether. To do this, I'd do the following and be done with it.
// MarshalText is implemented because ...
func (e *T) MarshalText() (text []byte, err error) {
var buf bytes.Buffer
if err := proto.MarshalText(&buf, e.InternalType); err != nil {
return nil, err
}
return buf.Bytes(), nil
}It'd then be good to write some tests of marshaling protos which use types and may or may not have populated user-defined type data to string.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @chengxiong-ruan and @postamar)
pkg/sql/types/types.go, line 228 at r1 (raw file):
// Convert PhysicalRepresentations to byte slice representation of string like // "PhysicalRepresentations<[0x12,0x34],[0x56]>". bf.WriteString("PhysicalRepresentations<")
If we're going to go for a prototext format, may as well go all the way. However,
309f765 to
2b0c053
Compare
chengxiong-ruan
left a comment
There was a problem hiding this comment.
good call out and thanks for the context! 👍
Reviewable status:
|
pkg/sql/types/types.go, line 228 at r1 (raw file): Previously, ajwerner wrote…
this part is removed :) |
2b0c053 to
79f8936
Compare
…face Fixes cockroachdb#63379 Having `types.T` to implement the `encoding.TextMarshaler` interface to avoid panics when gogo/protobuf tries to text marshal any protobuf struct which has direct/indirect `types.T` child. The panics was caused by non-protobuf types in `types.EnumMetadata`. This fix implements the `MarshalText` method to ignore user defined metadata. Only InternalType is dumped. Release note: None
79f8936 to
4271af4
Compare
|
pkg/sql/opt/exec/execbuilder/testdata/show_trace_nonmetamorphic, line 42 at r3 (raw file):
these logic test case were updated with |
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 2 of 3 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status:
|
bors r+ |
|
Build succeeded: |
Fixes #63379
types.EnumMetadataneeds to implementencoding.TextMarshalerinterface so that goto/protobuf won't panic when text marshaling
protobuf struct has child field of type
types.EnumMetadata.See the issue for more details why it would fail without this
fix.
Release note: None