github-actions[bot] commented on code in PR #40156:
URL: https://github.com/apache/beam/pull/40156#discussion_r4356159210
##########
sdks/go/pkg/beam/core/runtime/graphx/schema/logicaltypes.go:
##########
@@ -361,3 +361,34 @@ func checkLogicalTypeKinds(lt LogicalType) {
panic(fmt.Sprintf("LogicalType[%v] has the pointer storage type
%v, storage types must not be pointers", lt.ID(), lt.StorageType()))
}
}
+
+// RegisterPassThroughLogicalType registers a logical type identified by urn
+// whose values are used as their representation type. A schema field with
+// the urn maps to goType and uses the row encoding of goType, and any
+// argument of the logical type is ignored. Go values of goType are not mapped
+// back to the logical type, so a struct with a goType field produces a schema
+// field of the representation type. This suits the standard fixed and
+// variable length string and bytes logical types, whose argument only
+// constrains the length.
+//
+// RegisterPassThroughLogicalType must be called before beam.Init(), and
+// conventionally is called in a package init() function.
+func RegisterPassThroughLogicalType(urn string, goType reflect.Type) {
+ defaultRegistry.RegisterPassThroughLogicalType(urn, goType)
+}
+
+// RegisterPassThroughLogicalType registers a logical type whose values are
+// used as their representation type. See the package level function.
+func (r *Registry) RegisterPassThroughLogicalType(urn string, goType
reflect.Type) {
+ if len(urn) == 0 {
+ panic(fmt.Sprintf("invalid pass through logical type, bad urn
for %v", goType))
+ }
+ if _, err := r.reflectTypeToFieldType(goType); err != nil {
+ panic(fmt.Sprintf("pass through LogicalType[%v] has an invalid
type %v: %v", urn, goType, err))
+ }
+ lt := ToLogicalType(urn, goType, goType)
+ if prev, ok := r.logicalTypes[lt.key()]; ok && !prev.equals(lt) {
Review Comment:
**🟡 Medium**: `RegisterPassThroughLogicalType` currently accepts pointer
types (e.g., `*string`) because `r.reflectTypeToFieldType` succeeds on pointers
(setting `Nullable: true`). However, `fieldTypeToReflectType` wraps
`lt.GoType()` in `reflect.PtrTo` only when `sft.GetNullable()` is true, and the
row coder determines field nullability from whether the struct field type is a
pointer. If a pointer type is registered as `goType`, a non-nullable logical
type field would map to `*T` (causing the row coder to expect a nullability bit
not present in the wire format), and a nullable field would map to `**T`.
Call `checkLogicalTypeKinds(lt)` (or explicitly disallow `goType.Kind() ==
reflect.Ptr`) before registering `lt`.
```suggestion
lt := ToLogicalType(urn, goType, goType)
checkLogicalTypeKinds(lt)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
