Encode paneinfo with PaneInfoCoder. by claudevdm · Pull Request #34824 · apache/beam

claudevdm · 2025-05-02T22:54:31Z

Please add a meaningful description for your change here

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

github-actions · 2025-05-03T00:52:32Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

github-actions · 2025-05-03T15:35:31Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @liferoad for label python.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

shunping

Just out of curiosity, what's wrong with not having a PaneInfoCoder. I see the issue #34826, but does FastPrimitiveCoder still work?

sdks/python/apache_beam/coders/coders.py

claudevdm · 2025-05-04T22:33:39Z

Just out of curiosity, what's wrong with not having a PaneInfoCoder. I see the issue #34826, but does FastPrimitiveCoder still work?

FastPrimitiveCoder ends up falling back to PickleCoder and much bigger encodings. PaneInfoCoder encodes it using 1 byte.

shunping · 2025-05-05T02:39:58Z

Just out of curiosity, what's wrong with not having a PaneInfoCoder. I see the issue #34826, but does FastPrimitiveCoder still work?

FastPrimitiveCoder ends up falling back to PickleCoder and much bigger encodings. PaneInfoCoder encodes it using 1 byte.

I don't get why we need to expose PaneInfoCoder because I don't think we want cx to use it (or mess with it). PaneInfoCoder should implicitly be part of WindowValueCoder according to runner api proto (

beam/model/pipeline/src/main/proto/org/apache/beam/model/pipeline/v1/beam_runner_api.proto

Line 938 in c9a0e90

// pane - The first byte of the pane info determines which type of

).

I may be missing some important piece of background in #34348, but regarding

beam/sdks/python/apache_beam/transforms/util.py

Line 955 in c9a0e90

return key, (value, timestamp, pane_info)

can we do the following?

return key, window.GlobalWindows.windowed_value(value, timestamp, pane_info)

In this case, the coder of this output will be KVCoder[KeyCoder, WindowedValueCoder[GlobalWindowCoder, ValueCoder]], instead of KVCoder[KeyCoder, TupleCoder[ValueCoder, TimeStampCoder, PaneInfoCoder]].

claudevdm · 2025-05-05T12:17:27Z

Just out of curiosity, what's wrong with not having a PaneInfoCoder. I see the issue #34826, but does FastPrimitiveCoder still work?

FastPrimitiveCoder ends up falling back to PickleCoder and much bigger encodings. PaneInfoCoder encodes it using 1 byte.

I don't get why we need to expose PaneInfoCoder because I don't think we want cx to use it (or mess with it). PaneInfoCoder should implicitly be part of WindowValueCoder according to runner api proto (

beam/model/pipeline/src/main/proto/org/apache/beam/model/pipeline/v1/beam_runner_api.proto

Line 938 in c9a0e90

// pane - The first byte of the pane info determines which type of

).
I may be missing some important piece of background in #34348, but regarding

beam/sdks/python/apache_beam/transforms/util.py

Line 955 in c9a0e90

return key, (value, timestamp, pane_info)

can we do the following?
return key, window.GlobalWindows.windowed_value(value, timestamp, pane_info)
In this case, the coder of this output will be KVCoder[KeyCoder, WindowedValueCoder[GlobalWindowCoder, ValueCoder]], instead of KVCoder[KeyCoder, TupleCoder[ValueCoder, TimeStampCoder, PaneInfoCoder]].

That logic specifically avoids the window param for performance reasons #4933.

See the comments

# In this (common) case we can use a trivial trigger driver
# and avoid the (expensive) window param.

shunping · 2025-05-05T13:54:41Z

Just out of curiosity, what's wrong with not having a PaneInfoCoder. I see the issue #34826, but does FastPrimitiveCoder still work?

FastPrimitiveCoder ends up falling back to PickleCoder and much bigger encodings. PaneInfoCoder encodes it using 1 byte.

I don't get why we need to expose PaneInfoCoder because I don't think we want cx to use it (or mess with it). PaneInfoCoder should implicitly be part of WindowValueCoder according to runner api proto (

beam/model/pipeline/src/main/proto/org/apache/beam/model/pipeline/v1/beam_runner_api.proto

Line 938 in c9a0e90

// pane - The first byte of the pane info determines which type of

).
I may be missing some important piece of background in #34348, but regarding

beam/sdks/python/apache_beam/transforms/util.py

Line 955 in c9a0e90

return key, (value, timestamp, pane_info)

can we do the following?
return key, window.GlobalWindows.windowed_value(value, timestamp, pane_info)
In this case, the coder of this output will be KVCoder[KeyCoder, WindowedValueCoder[GlobalWindowCoder, ValueCoder]], instead of KVCoder[KeyCoder, TupleCoder[ValueCoder, TimeStampCoder, PaneInfoCoder]].
That logic specifically avoids the window param for performance reasons #4933.

See the comments
# In this (common) case we can use a trivial trigger driver
# and avoid the (expensive) window param.

Hmmmm, my understanding of that comment is different. I think the optimization in #4933 is that for global windows we don't need to put WindowParam in the DoFn. Anyway, just want to call out here.

def reify_timestamps(element, timestamp=DoFn.TimestampParam):
   ...

Abacn · 2025-05-05T15:06:12Z

sdks/python/apache_beam/coders/typecoders.py

    self._register_coder_internal(bytes, coders.BytesCoder)
    self._register_coder_internal(bool, coders.BooleanCoder)
    self._register_coder_internal(str, coders.StrUtf8Coder)
+    self._register_coder_internal(windowed_value.PaneInfo, coders.PaneInfoCoder)


This fix has a large blast radius. It changed registered standard coders which is default behavior globally. Shall we do this or just put a coder implementation in new ReShuffle?

Are you suggesting to define PaneInfoCoder here

beam/sdks/python/apache_beam/transforms/util.py

Line 996 in 6f2f960

and then use CoderRegistry.register_coder?

I don't have strong opinion, is there a disadvantage to doing it here? It seems like an 'internal' coder use case?

@kennknowles What do you think about this? We should use PaneInfoCoder but it could break the job update.

is there a disadvantage to doing it here

This introduces a breaking change such that windowed_value.PaneInfo has changed default coder. This is not limited to ReShuffle but also user pipeline.

Since test results shows PaneInfoCoder is much more efficient than FastPrimitiveCoder for PaneInfo, I am fine with the change, just need to know this is another breaking change introduced in 2.65.0

If we keep this change I recommend to also mention this in https://github.com/apache/beam/blob/master/CHANGES.md#breaking-changes-1

Yes, we should update https://github.com/apache/beam/blob/master/CHANGES.md#breaking-changes-1 at least.

shunping · 2025-05-05T15:47:03Z

Just out of curiosity, what's wrong with not having a PaneInfoCoder. I see the issue #34826, but does FastPrimitiveCoder still work?

FastPrimitiveCoder ends up falling back to PickleCoder and much bigger encodings. PaneInfoCoder encodes it using 1 byte.

I don't get why we need to expose PaneInfoCoder because I don't think we want cx to use it (or mess with it). PaneInfoCoder should implicitly be part of WindowValueCoder according to runner api proto (

beam/model/pipeline/src/main/proto/org/apache/beam/model/pipeline/v1/beam_runner_api.proto

Line 938 in c9a0e90

// pane - The first byte of the pane info determines which type of

).
I may be missing some important piece of background in #34348, but regarding

beam/sdks/python/apache_beam/transforms/util.py

Line 955 in c9a0e90

return key, (value, timestamp, pane_info)

can we do the following?
return key, window.GlobalWindows.windowed_value(value, timestamp, pane_info)
In this case, the coder of this output will be KVCoder[KeyCoder, WindowedValueCoder[GlobalWindowCoder, ValueCoder]], instead of KVCoder[KeyCoder, TupleCoder[ValueCoder, TimeStampCoder, PaneInfoCoder]].
That logic specifically avoids the window param for performance reasons #4933.
See the comments
# In this (common) case we can use a trivial trigger driver
# and avoid the (expensive) window param.
Hmmmm, my understanding of that comment is different. I think the optimization in #4933 is that for global windows we don't need to put WindowParam in the DoFn. Anyway, just want to call out here.
def reify_timestamps(element, timestamp=DoFn.TimestampParam):
   ...

CC @robertwb (the author of $4933) regarding the WindowParam comment here.

* Encode paneinfo with PaneInfoCoder. * Fix tests. * Fix import. * Add typecoder test. * Implement eq and hash. --------- Co-authored-by: Claude <cvandermerwe@google.com>

github-actions bot added the python label May 2, 2025

Encode paneinfo with PaneInfoCoder.

62b2c44

claudevdm force-pushed the fix-paneinfo-encoding branch from a51cee8 to 62b2c44 Compare May 2, 2025 22:55

Fix tests.

3305c8f

claudevdm mentioned this pull request May 2, 2025

[Bug]: Reshuffle with default windows encodes PaneInfo with FastPrmitivesCoder #34826

Closed

17 tasks

Claude added 2 commits May 3, 2025 13:50

Fix import.

0082b84

Add typecoder test.

fc49666

github-actions bot added the Next Action: Reviewers label May 3, 2025

claudevdm requested review from Abacn and kennknowles May 3, 2025 15:57

shunping reviewed May 3, 2025

View reviewed changes

sdks/python/apache_beam/coders/coders.py Show resolved Hide resolved

Implement eq and hash.

6bba980

liferoad approved these changes May 5, 2025

View reviewed changes

liferoad merged commit ff9a22a into apache:master May 5, 2025
90 checks passed

Abacn reviewed May 5, 2025

View reviewed changes

Conversation

claudevdm commented May 2, 2025

GitHub Actions Tests Status (on master branch)

Uh oh!

github-actions bot commented May 3, 2025

Uh oh!

github-actions bot commented May 3, 2025

Uh oh!

shunping left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

claudevdm commented May 4, 2025

Uh oh!

shunping commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claudevdm commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

shunping commented May 5, 2025

Uh oh!

Abacn May 5, 2025

Choose a reason for hiding this comment

Uh oh!

claudevdm May 5, 2025

Choose a reason for hiding this comment

Uh oh!

liferoad May 5, 2025

Choose a reason for hiding this comment

Uh oh!

Abacn May 5, 2025

Choose a reason for hiding this comment

Uh oh!

liferoad May 5, 2025

Choose a reason for hiding this comment

Uh oh!

shunping commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shunping commented May 5, 2025 •

edited

Loading

claudevdm commented May 5, 2025 •

edited

Loading

shunping commented May 5, 2025 •

edited

Loading