(cleanup) marshal: remove proto field from NativeType struct#712
(cleanup) marshal: remove proto field from NativeType struct#712mykaul wants to merge 4 commits intoscylladb:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Removes the unused protocol-version field from NativeType to reduce per-type memory overhead and simplify related parsing/marshaling code paths.
Changes:
- Drops
NativeType.protoand updates type construction sites accordingly (prod + tests). - Makes
NativeType.Version()always returnprotoVersion4and removes theMarshal()panic guard tied to unset proto. - Simplifies internal helpers/signatures by removing now-redundant protocol-version parameters (type parsing + tablet hint unmarshalling).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| marshal.go | Removes NativeType.proto, updates constructors/Version behavior, and adjusts UDT construction accordingly. |
| helpers.go | Removes proto-version from internal Cassandra type parsing helpers and updates vector parsing to match. |
| frame.go | Updates framer.readTypeInfo() type-info construction to no longer set proto. |
| conn.go | Updates tablet hint unmarshalling to no longer accept/pass a proto version and removes proto from nested type literals. |
| marshal_test.go | Updates marshaling/unmarshaling tests to remove proto: from NativeType literals. |
| ring_describer_test.go | Updates result metadata fixtures to remove proto: from NativeType literals. |
| helpers_test.go | Updates tests for the new getCassandraType signature. |
| tablet_integration_test.go | Updates integration test call site for the new unmarshalTabletHint signature. |
Comments suppressed due to low confidence (1)
marshal.go:1843
- NewUDTType still accepts a proto parameter but no longer uses it after removing NativeType.proto. This is a silent behavior change for callers; please document that the proto argument is ignored (and ideally deprecate it with a leading "Deprecated:" sentence) to avoid confusion.
func NewUDTType(proto byte, name, keySpace string, elems ...UDTField) UDTTypeInfo {
return UDTTypeInfo{
NativeType: NativeType{typ: TypeUDT},
Name: name,
KeySpace: keySpace,
Elements: elems,
}
| // NewNativeType creates a NativeType. The proto parameter is ignored and kept for API compatibility. | ||
| func NewNativeType(proto byte, typ Type) NativeType { | ||
| return NativeType{proto: proto, typ: typ, custom: ""} | ||
| return NativeType{typ: typ} | ||
| } | ||
|
|
||
| // NewCustomType creates a custom NativeType. The proto parameter is ignored and kept for API compatibility. | ||
| func NewCustomType(proto byte, typ Type, custom string) NativeType { | ||
| return NativeType{proto: proto, typ: typ, custom: custom} | ||
| return NativeType{typ: typ, custom: custom} | ||
| } |
There was a problem hiding this comment.
NewNativeType/NewCustomType now ignore the proto parameter, but the doc comments don’t mark these APIs as deprecated (Go convention is a leading "Deprecated:" sentence). Either update the comments to use the standard deprecation marker or adjust the PR description/intent so users aren’t misled by godoc.
There was a problem hiding this comment.
@copilot - this is slightly wrong - if I use 'Deprecated:' here, one might think that the func is deprecated, whereas just a single parameter is. Which is why I've added a comment above each of them.
There is the exact same problem as in Python, right? This functionality is not strictly needed now, but will be in the future when we support new protocol versions (especially if ser/deser for some types changes). |
I thought about this, but:
|
mykaul
force-pushed
the
remove_proto_from_native_type
branch
2 times, most recently
from
f97a0bb to
53ea7d1
Compare
mykaul
force-pushed
the
remove_proto_from_native_type
branch
from
53ea7d1 to
893b131
Compare
Remove the functionally unused proto byte field from NativeType, reducing struct size from 32 to 24 bytes (1 byte + 7 padding). Reduces memory consumption and thus GC. Changes: - Remove proto byte field from NativeType struct - Version() now returns protoVersion4 constant, marked deprecated - NewNativeType/NewCustomType accept but ignore proto param, marked deprecated - Remove Marshal panic guard that checked proto == 0 - Remove proto: from all struct literals across production and test files - Remove protoVer parameter from internal getCassandraType/getCassandraLongType - Remove unused v uint8 parameter from unmarshalTabletHint All unit tests passed, ran Scylla integration tests as well. Signed-off-by: Yaniv Kaul <yaniv.kaul@scylladb.com>
mykaul
force-pushed
the
remove_proto_from_native_type
branch
from
893b131 to
5fc9efa
Compare
The proto field was removed from NativeType in this PR but events_unit_test.go was not updated, causing a compilation failure.
Use the conventional '// Deprecated:' format so that Go tooling (godoc, staticcheck, gopls) can surface the deprecation to users.
Remove the functionally unused proto byte field from NativeType, reducing struct size from 32 to 24 bytes (1 byte + 7 padding). Reduces memory consumption and thus GC.
Changes:
All unit tests passed, ran Scylla integration tests as well.