scwhittle commented on code in PR #36830:
URL: https://github.com/apache/beam/pull/36830#discussion_r2533268122
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/windowing/PaneInfo.java:
##########
@@ -411,5 +413,20 @@ public PaneInfo decode(final InputStream inStream) throws
CoderException, IOExce
@Override
public void verifyDeterministic() {}
+
+ @Override
+ protected long getEncodedElementByteSize(PaneInfo value) throws Exception {
Review Comment:
can you add some unit tests creating various elements and comparing this and
actual serialized size?
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/ValueWithRecordId.java:
##########
@@ -124,6 +127,13 @@ public void verifyDeterministic() throws
NonDeterministicException {
valueCoder.verifyDeterministic();
}
+ @Override
+ public void registerByteSizeObserver(
Review Comment:
should we implement the isCheap method as well, delegating to the coders?
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/windowing/PaneInfo.java:
##########
@@ -411,5 +413,20 @@ public PaneInfo decode(final InputStream inStream) throws
CoderException, IOExce
@Override
public void verifyDeterministic() {}
+
+ @Override
Review Comment:
nit: move method next to encode? more likely to see they both need to be
modified
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/windowing/PaneInfo.java:
##########
@@ -411,5 +413,20 @@ public PaneInfo decode(final InputStream inStream) throws
CoderException, IOExce
@Override
public void verifyDeterministic() {}
+
+ @Override
Review Comment:
can you update PR description? It says we are encoding for every element to
calculate the size, but since the cheap method returns false it should just be
sampled elements.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/windowing/IntervalWindow.java:
##########
@@ -186,8 +186,7 @@ public boolean consistentWithEquals() {
@Override
public boolean isRegisterByteSizeObserverCheap(IntervalWindow value) {
- return instantCoder.isRegisterByteSizeObserverCheap(value.end)
- && durationCoder.isRegisterByteSizeObserverCheap(new
Duration(value.start, value.end));
+ return true;
Review Comment:
should we inline the DurationCoder logic and remove it? It is just varint
and we could then avoid creating Duration objects
--
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]
