lostluck commented on code in PR #25203:
URL: https://github.com/apache/beam/pull/25203#discussion_r1089572178
##########
sdks/go/pkg/beam/core/funcx/sideinput.go:
##########
@@ -91,6 +91,9 @@ func unfoldIter(t reflect.Type) ([]reflect.Type, bool, error)
{
if ok, err := isOutParam(t.In(i)); !ok {
return nil, false, errors.Wrap(err,
errIllegalParametersInIter)
}
+ if t.In(i).Elem().Kind() == reflect.Interface{
+ panic("Type interface{} isn't a supported PCollection
type")
Review Comment:
Same here, don't panic, check for something more specific.
##########
sdks/go/pkg/beam/core/funcx/output.go:
##########
@@ -84,6 +84,9 @@ func unfoldEmit(t reflect.Type) ([]reflect.Type, bool, error)
{
if ok, err := isInParam(t.In(i)); !ok {
return nil, false, errors.Wrap(err,
errIllegalParametersInEmit)
}
+ if t.In(i).Kind() == reflect.Interface || (t.In(i).Kind() ==
reflect.Pointer && t.In(i).Elem().Kind() == reflect.Interface) {
Review Comment:
Checking the Kind() against reflect.Interface is a good first approximation,
but this prevents *all* interfaces from appearing in types in beam.
The problem the issue needs to address isn't "no interfaces are allowed
ever" but "the vanilla empty interface{}" isn't allowed.
But care needs to be taken: Not all "empty interfaces" are created equally.
Since BeamGo was developed before Go Generics, we needed some way of simulating
that kind of behavior. We called these "Universal Types".
Universal types are lightly described in the design RFC here:
https://s.apache.org/beam-go-sdk-design-rfc#heading=h.1vovlxln20bf
You can detect universal types with the function `typex.IsUniversal()`:
https://github.com/apache/beam/blob/master/sdks/go/pkg/beam/core/typex/class.go#L224
Look at how "IsUniversal" is defined, and how the different universal types
are defined.
----
Further, we do allow arbitary interface types in these various function type
signatures, but we require them to have a coder registered with Beam. That
validation necessarily happens at a later stage, to avoid unnecessary coupling
and complications at early parts of validation.
Since the empty interface can't have a coder defined for it (everything
satisfies the empty interface), it's simpler to not allow it.
----
Then you can see that what we want to do for this bug is compare the type
against the type of the empty interface.
```
var anyType = reflect.TypeOf((*any)(nil)).Elem()
```
(`any` is an alias for `interface{}`).
Let me know if you have questions about this.
##########
sdks/go/pkg/beam/core/funcx/output.go:
##########
@@ -84,6 +84,9 @@ func unfoldEmit(t reflect.Type) ([]reflect.Type, bool, error)
{
if ok, err := isInParam(t.In(i)); !ok {
return nil, false, errors.Wrap(err,
errIllegalParametersInEmit)
}
+ if t.In(i).Kind() == reflect.Interface || (t.In(i).Kind() ==
reflect.Pointer && t.In(i).Elem().Kind() == reflect.Interface) {
+ panic("Type interface{} isn't a supported PCollection
type")
Review Comment:
If there's an error return, one should return an error instead of panicking.
Panicking is generally a last resort thing, and rarely when one can return an
error instead.
##########
sdks/go/pkg/beam/io/parquetio/parquetio.go:
##########
@@ -149,7 +149,7 @@ type parquetWriteFn struct {
Filename string `json:"filename"`
}
-func (a *parquetWriteFn) ProcessElement(ctx context.Context, _ int, iter
func(*any) bool) error {
+func (a *parquetWriteFn) ProcessElement(ctx context.Context, _ int, iter
func(*beam.X) bool) error {
Review Comment:
This is the correct fix! No notes here.
--
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]
