Skip to content

[Go SDK] Propagate paneInfo correctly.#36191

Merged
shunping merged 5 commits intoapache:masterfrom
shunping:issue-31153
Sep 18, 2025
Merged

[Go SDK] Propagate paneInfo correctly.#36191
shunping merged 5 commits intoapache:masterfrom
shunping:issue-31153

Conversation

@shunping
Copy link
Copy Markdown
Collaborator

@shunping shunping commented Sep 18, 2025 •
edited
Loading

fixes #31153

Rather the default pane mentioned in the bug (also shown below),

got pane {Timing:0 IsFirst:false IsLast:false Index:0 NonSpeculativeIndex:0}

we get the NoFiringPane after the fix.

"got pane {Timing:unknown IsFirst:true IsLast:true Index:0 NonSpeculativeIndex:0}"

@github-actions github-actions bot added the go label Sep 18, 2025
@shunping shunping marked this pull request as ready for review September 18, 2025 13:09
@shunping shunping self-assigned this Sep 18, 2025
@shunping
Copy link
Copy Markdown
Collaborator Author

shunping commented Sep 18, 2025

Looks like I need to generate those shims.go and some files under pkg/beam/core/runtime/exec/optimized/.

Do we have any instruction to do that? @lostluck

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 18, 2025

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

@shunping shunping force-pushed the issue-31153 branch 4 times, most recently from dc0004e to 207160e Compare September 18, 2025 14:49
@shunping
Copy link
Copy Markdown
Collaborator Author

shunping commented Sep 18, 2025 •
edited
Loading

Ok. I ran go generate under the following affected packages/folders:

sdks/go/pkg/beam
sdks/go/pkg/beam/testing/passert
sdks/go/pkg/beam/core/runtime/exec/optimized
sdks/go/pkg/beam/x/debug
sdks/go/pkg/beam/runners/vet/testpipeline

However, it seems there more diffs than I would expected. Now I am not sure if I did it correctly ...

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Sep 18, 2025

Assigning reviewers:

R: @lostluck for label go.

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

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).

@lostluck
Copy link
Copy Markdown
Contributor

lostluck commented Sep 18, 2025

Ok. I ran go generate under the following affected packages/folders:

sdks/go/pkg/beam
sdks/go/pkg/beam/testing/passert
sdks/go/pkg/beam/core/runtime/exec/optimized
sdks/go/pkg/beam/x/debug
sdks/go/pkg/beam/runners/vet/testpipeline

However, it seems there more diffs than I would expected. Now I am not sure if I did it correctly ...

The code generator has been largely deprecated for the last 3 years, in favour of generic registration package (...beam/register), so it's most likely those diffs are due to not having been updated since then. It's definitely a thing where fixing the generator is better than manually fixing the diffs.

Taking a look...

lostluck
lostluck approved these changes Sep 18, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see the diffs are largely around the beam.T vs the typex.T distinction. IIRC the generation was updated at some point to not chase the types as hard, and that's lead to this difference. That was partly to fix some issues with the use of the generator inside google, thanks to the Go team.

LGTM.

@codecov
Copy link
Copy Markdown

codecov bot commented Sep 18, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 59.90338% with 83 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.77%. Comparing base (d0e48e2) to head (9d29312).
⚠️ Report is 32 commits behind head on master.

Files with missing lines Patch % Lines
sdks/go/pkg/beam/beam.shims.go 44.21% 53 Missing ⚠️
sdks/go/pkg/beam/testing/passert/passert.shims.go 68.75% 15 Missing ⚠️
sdks/go/pkg/beam/x/debug/debug.shims.go 82.97% 8 Missing ⚠️
sdks/go/pkg/beam/core/runtime/exec/pardo.go 20.00% 1 Missing and 3 partials ⚠️
...eam/runners/vet/testpipeline/testpipeline.shims.go 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##             master   #36191   +/-   ##
=========================================
  Coverage     56.76%   56.77%           
  Complexity     3384     3384           
=========================================
  Files          1220     1220           
  Lines        185448   185547   +99     
  Branches       3520     3520           
=========================================
+ Hits         105277   105345   +68     
- Misses        76843    76869   +26     
- Partials       3328     3333    +5
Flag Coverage Δ
go 28.22% <59.90%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@shunping shunping merged commit c1618ba into apache:master Sep 18, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lostluck lostluck lostluck approved these changes

Assignees

@shunping shunping

Labels

go Next Action: Reviewers runners

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: PaneInfo not populated in Go SDK

2 participants

@shunping @lostluck