lostluck commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1030707840
##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -245,6 +245,10 @@ func (b *CoderUnmarshaller) makeCoder(id string, c
*pipepb.Coder) (*coder.Coder,
t := typex.New(root,
append([]typex.FullType{key.T}, coder.Types(values)...)...)
return &coder.Coder{Kind: kind, T: t,
Components: append([]*coder.Coder{key}, values...)}, nil
}
+ case urnIntervalWindow:
+ // If interval window in a KV, this may be a mapping
function.
+ // Special case since windows are not normally used
directly as FullValues.
+ return coder.NewIntervalWindowCoder(), nil
Review Comment:
Basically, the Iterable/StateBackedIterable cases need special handling
because technically CoGBK results are KV<k, Iter<V>> (or even KV<k, Iter<V1>,
... , Iter<Vn>>), but we avoid degenerate KVs with more than 2 components in
the Go SDK, by having a dedicated CoGBK "kind" for coders.
This lets us treat the data ingestion properly datasource.go, and be able to
do the special handling to convert to the pull iterator functions `func(*V)
bool` (and with any luck, once Go settles on an Iterator convention, support
that style as well.)
Basically, if we had "real" KV support as a single KV element instead of
splitting K and V parameters, we wouldn't need to go this far, but we're stuck
with supporting it in the current API.
But this is a long way of saying, we don't need to special case things at
this layer since there's nothing "tricky" about the element type technically,
we just put Windows into their own box to avoid accidental mistakes (or
generally nonsensical user mistakes like "processing" the internal windows
metadata)
--
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]
