sql/types: types.EnumMetadata implements encoding.TextMarshaler interface #72640
Conversation
|
This change is |
chengxiong-ruan
force-pushed
the
enummetadata_implements_textmarshaler_interface
branch
from
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... |
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,
chengxiong-ruan
force-pushed
the
enummetadata_implements_textmarshaler_interface
branch
from
309f765 to
2b0c053
Compare
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 :) |
chengxiong-ruan
force-pushed
the
enummetadata_implements_textmarshaler_interface
branch
from
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
chengxiong-ruan
force-pushed
the
enummetadata_implements_textmarshaler_interface
branch
from
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 |
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: |
chengxiong-ruan
deleted the
enummetadata_implements_textmarshaler_interface
branch
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