shunping commented on code in PR #31092:
URL: https://github.com/apache/beam/pull/31092#discussion_r1584948151
##########
model/fn-execution/src/main/proto/org/apache/beam/model/fn_execution/v1/beam_fn_api.proto:
##########
@@ -1063,6 +1087,10 @@ message StateAppendRequest {
// Represents a part of a logical byte stream. Elements within
// the logical byte stream are encoded in the nested context and
// multiple append requests are concatenated together.
+ // For OrderedListState, elements of TimeStampedValue<T> should be encoded
+ // with TimestampedValueCoder.of(LengthPrefixCoder.of(Coder<T>)), so that
+ // the request handler knows how to decode timestamps from the data without
Review Comment:
> Looks like TimestampedValueCoder is not well defined
TimestampedValueCoder is defined as an inner class in `TimeStampedValue`
https://github.com/apache/beam/blob/413af1289373079319ba77b3f233751fbdfc3bc9/sdks/java/core/src/main/java/org/apache/beam/sdk/values/TimestampedValue.java#L88
. It encodes the value first and then the timestamp.
> Should we use KV<VarInt, LP<value>>? Or is it worth defining a bigendian
one? How do we want to treat negatives?
I use TimeStampedValue<T> in the proto and implementation instead of a KV<,>
because it is originally used in the OrderedListState interface
https://github.com/apache/beam/blob/f0d06051072da5b64c496dc8456e56bab9aca71b/sdks/java/core/src/main/java/org/apache/beam/sdk/state/OrderedListState.java#L29.
Another benefit of using TimeStampedValue<T> is that we can reuse all the
existing prefetched mechanisms without reimplementing them.
Here is a [long discussion
thread](https://docs.google.com/document/d/1U77sAvE6Iy9XsVruRYHxPdFji7nqS6HPi1XU8fhyrxs/edit?disco=AAAAHwml1Yg)
of why we choose to use timestamp as the sort key in the original design doc.
As Reuven pointed out, timestamp is the most common sort key and we can easily
converted it from and to an int64.
```orderedList.add(TimestampedValue.of(element, Instant.of(sequence
number)));```
This is valid for both positives and negatives (which will be represented as
timestamps prior to epoch 1970-01-01T00:00:00Z).
--
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]
