lostluck commented on code in PR #23222:
URL: https://github.com/apache/beam/pull/23222#discussion_r971099015
##########
sdks/go/pkg/beam/core/typex/special.go:
##########
@@ -88,6 +89,23 @@ type PaneInfo struct {
Index, NonSpeculativeIndex int64
}
+type Timers struct {
Review Comment:
These structs are almost but not quite identical, it would be good to have a
comment for why the key fields are different.
Either way, please add doc comments for each: `// Timers ...` `// TimerMap
...` (filled in, rather than leaving it as `...`).
##########
sdks/go/pkg/beam/core/typex/special.go:
##########
@@ -36,6 +36,7 @@ var (
EventTimeType = reflect.TypeOf((*EventTime)(nil)).Elem()
WindowType = reflect.TypeOf((*Window)(nil)).Elem()
+ TimersType = reflect.TypeOf((*Timers)(nil)).Elem()
Review Comment:
This seems like the only place that `Timers` is used (at least in this PR).
Why can't this be TimersMap?
##########
sdks/go/pkg/beam/core/typex/special.go:
##########
@@ -88,6 +89,23 @@ type PaneInfo struct {
Index, NonSpeculativeIndex int64
}
+type Timers struct {
Review Comment:
Why are there two of the almost the same type? Since these are exported,
they need their own doc comments at least, but given their near-identical-ness,
an additional comment about why we need one and not the other would be valuable.
##########
sdks/go/pkg/beam/core/runtime/exec/coder.go:
##########
@@ -1208,3 +1217,92 @@ func DecodeWindowedValueHeader(dec WindowDecoder, r
io.Reader) ([]typex.Window,
return ws, t, pn, nil
}
+
+// encodeTimer encodes a typex.TimerMap into a byte stream.
+func encodeTimer(elm ElementEncoder, win WindowEncoder, tm typex.TimerMap, w
io.Writer) error {
+ var b bytes.Buffer
+
+ elm.Encode(&FullValue{Elm: tm.Key}, &b)
+
+ if err := coder.EncodeStringUTF8(tm.Tag, &b); err != nil {
+ return errors.WithContext(err, "error encoding tag")
+ }
+
+ if err := win.Encode(tm.Windows, &b); err != nil {
+ return errors.WithContext(err, "error encoding window")
+ }
+ if err := coder.EncodeBool(tm.Clear, &b); err != nil {
+ return errors.WithContext(err, "error encoding clear bit")
+ }
+
+ if !tm.Clear {
+ if err := coder.EncodeEventTime(tm.FireTimestamp, &b); err !=
nil {
+ return errors.WithContext(err, "error encoding fire
timestamp")
+ }
+ if err := coder.EncodeEventTime(tm.HoldTimestamp, &b); err !=
nil {
+ return errors.WithContext(err, "error encoding hold
timestamp")
+ }
+ if err := coder.EncodePane(tm.PaneInfo, &b); err != nil {
+ return errors.WithContext(err, "error encoding
paneinfo")
+ }
+ }
+
+ w.Write(b.Bytes())
+ return nil
+}
+
+// decodeTimer decodes timer byte encoded with standard timer coder spec.
+func decodeTimer(dec ElementDecoder, win WindowDecoder, r io.Reader)
(typex.TimerMap, error) {
+ tm := typex.TimerMap{}
+
+ if fv, err := dec.Decode(r); err != nil {
+ return tm, errors.WithContext(err, "error decoding timer key")
+ } else {
+ tm.Key = fv.Elm.(string)
+ }
+
+ if s, err := coder.DecodeStringUTF8(r); err != nil && err != io.EOF {
+ return tm, errors.WithContext(err, "error decoding tag")
+ } else if err == io.EOF {
+ tm.Tag = ""
+ } else {
+ // when tag is empty
+ tm.Tag = s
+ }
Review Comment:
While this structure is valid Go, it's also not intuitively correct, since
it relies on the lesser known if scoping rules to work.
My recommendation would be to *not* inline the reads with the if statement,
and drop the else block wrappings entirely. This keeps the "happy path" on the
leftmost indent. Right now it's hard to determine which is the error case, vs
not.
What you have is fine once in a while, but not as every block in a function.
```
s, err := coder.DecodeStringUTF8(r);
if err != nil && err != io.EOF {
return tm, errors.WithContext(err, "error decoding tag")
} else if err == io.EOF {
tm.Tag = ""
}
tm.Tag = s
```
##########
sdks/go/pkg/beam/core/typex/special.go:
##########
@@ -88,6 +89,23 @@ type PaneInfo struct {
Index, NonSpeculativeIndex int64
}
+type Timers struct {
+ Key []byte // elm type.
+ Tag string
+ Windows []byte // []typex.Window
+ Clear bool
+ FireTimestamp, HoldTimestamp mtime.Time
+ PaneInfo PaneInfo
+}
+
+type TimerMap struct {
+ Key, Tag string
+ Windows []Window // []typex.Window
+ Clear bool
+ FireTimestamp, HoldTimestamp mtime.Time
+ PaneInfo PaneInfo
Review Comment:
I'd just call field this Pane, rather than PaneInfo. Same for the Timers
field. As a rule, don't duplicate field names with type names. That usually
means there's something funny with the semantics of one or the other.
--
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]
