lostluck commented on a change in pull request #14192:
URL: https://github.com/apache/beam/pull/14192#discussion_r596200323
##########
File path: sdks/go/pkg/beam/core/runtime/graphx/schema/schema.go
##########
@@ -280,82 +301,194 @@ func (r *Registry) FromType(ot reflect.Type)
(*pipepb.Schema, error) {
return r.fromType(ot)
}
+func (r *Registry) logicalTypeToFieldType(t reflect.Type) (*pipepb.FieldType,
string, error) {
+ // Check if a logical type was registered that matches this struct type
directly
+ // and if so, extract the schema from it for use.
+ if lID, ok := r.logicalTypeIdentifiers[t]; ok {
+ lt := r.logicalTypes[lID]
+ ftype, err := r.reflectTypeToFieldType(lt.StorageType())
+ if err != nil {
+ return nil, "", errors.Wrapf(err, "unable to convert
LogicalType[%v]'s storage type %v for Go type of %v to a schema", lID,
lt.StorageType(), lt.GoType())
+ }
+ return ftype, lID, nil
+ }
+ for _, lti := range r.logicalTypeInterfaces {
+ if !t.Implements(lti) {
+ continue
+ }
+ p := r.logicalTypeProviders[lti]
+ st, err := p(t)
+ if err != nil {
+ return nil, "", errors.Wrapf(err, "unable to convert
LogicalType[%v] using provider for %v schema field", t, lti)
+ }
+ if st == nil {
+ continue
+ }
+ ftype, err := r.reflectTypeToFieldType(st)
+ if err != nil {
+ return nil, "", errors.Wrapf(err, "unable to convert
LogicalType[%v]'s storage type %v for Go type of %v to a schema", "interface",
st, t)
+ }
+ return ftype, t.String(), nil
+ }
+ return nil, "", nil
+}
+
func (r *Registry) fromType(ot reflect.Type) (*pipepb.Schema, error) {
- if reflectx.SkipPtr(ot).Kind() != reflect.Struct {
- return nil, errors.Errorf("cannot convert %v to schema.
FromType only converts structs to schemas", ot)
+ if schm, ok := r.typeToSchema[ot]; ok {
+ return schm, nil
}
- schm, err := r.structToSchema(ot)
+ ftype, lID, err := r.logicalTypeToFieldType(ot)
if err != nil {
return nil, err
}
- if ot.Kind() == reflect.Ptr {
- schm.Options = append(schm.Options, &pipepb.Option{
- Name: optGoNillable,
- })
+ if ftype != nil {
Review comment:
Very similar means they aren't the same. Also, structToSchema *always
and only* handles structs, not pointers to structs. If a pointer type is
registered for special handling, structToSchema will *never* catch it.
Originally structToSchema handled both, but that lead to *so many bugs*,
which this current approach avoids. You're right that there's probably a
further reduction opportunity here, but the cleanups done here already are
already far enough from what I have in my internal client, that I want to limit
further drift now.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
