You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/08/25 20:20:18 UTC

[GitHub] [beam] damccorm opened a new pull request, #22897: Add map state in the Go Sdk

damccorm opened a new pull request, #22897:
URL: https://github.com/apache/beam/pull/22897

   This continues to grow our state support by adding map state support. Once this is in, I'll add a follow up PR with an integration test. In the meantime, I tested manually by running the hacked wordcount found here -  - and it produced output like:
   
   ```
   any, 1. Total num keys: 2. With keys: [any5 any]
   any, 2. Total num keys: 3. With keys: [any7 any5 any]
   any, 3. Total num keys: 4. With keys: [any28 any7 any5 any]
   any, 4. Total num keys: 5. With keys: [any28 any7 any5 any any13]
   any, 5. Total num keys: 6. First 5 keys: [any28 any7 any5 any any13]
   any, 6. Total num keys: 7. First 5 keys: [any28 any7 any5 any any13]
   any, 7. Total num keys: 8. First 5 keys: [any28 any5 any13 any97 any98]
   any, 8. Total num keys: 9. First 5 keys: [any28 any5 any13 any97 any98]
   any, 9. Total num keys: 10. First 5 keys: [any28 any5 any13 any97 any98]
   any, 10. Total num keys: 11. First 5 keys: [any28 any5 any13 any97 any98]
   any, 11. Total num keys: 12. First 5 keys: [any28 any5 any13 any97 any98]
   any, 12. Total num keys: 13. First 5 keys: [any28 any5 any13 any97 any98]
   any, 13. Total num keys: 14. First 5 keys: [any28 any5 any13 any97 any98]
   any, 14. Total num keys: 14. First 5 keys: [any28 any5 any13 any97 any98]
   any, 15. Total num keys: 15. First 5 keys: [any33 any8 any28 any5 any6]
   any, 16. Total num keys: 16. First 5 keys: [any33 any8 any28 any5 any6]
   any, 17. Total num keys: 17. First 5 keys: [any33 any9 any8 any28 any5]
   any, 18. Total num keys: 18. First 5 keys: [any33 any9 any8 any28 any5]
   Camelot, 1. Total num keys: 2. With keys: [Camelot Camelot17]
   Mean, 1. Total num keys: 2. With keys: [Mean87 Mean]
   Reverbs, 1. Total num keys: 2. With keys: [Reverbs Reverbs73]
   devour, 1. Total num keys: 2. With keys: [devour devour26]
   vengeance, 1. Total num keys: 2. With keys: [vengeance vengeance6]
   vengeance, 2. Total num keys: 3. With keys: [vengeance72 vengeance vengeance6]
   vengeance, 3. Total num keys: 4. With keys: [vengeance64 vengeance6 vengeance72 vengeance]
   ...
   ```
   
   Note that this doesn't add support for Remove (to remove a single key). This will come in a future pr and has been noted in #22736
   
   Part of #22736
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
    - [ ] 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](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/get-started-contributing/#make-the-reviewers-job-easier).
   
   To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
   
   GitHub Actions Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Go tests](https://github.com/apache/beam/workflows/Go%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
   
   See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
   


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] github-actions[bot] commented on pull request #22897: Add map state in the Go Sdk

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #22897:
URL: https://github.com/apache/beam/pull/22897#issuecomment-1227739568

   Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] damccorm commented on a diff in pull request #22897: Add map state in the Go Sdk

Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #22897:
URL: https://github.com/apache/beam/pull/22897#discussion_r956327308


##########
sdks/go/pkg/beam/core/runtime/graphx/translate.go:
##########
@@ -1376,3 +1398,13 @@ func UpdateDefaultEnvWorkerType(typeUrn string, pyld []byte, p *pipepb.Pipeline)
 	}
 	return errors.Errorf("unable to find dependency with %q role in environment with ID %q,", URNArtifactGoWorkerRole, defaultEnvId)
 }
+
+// UserStateCoderId returns the coder id of a user state
+func UserStateCoderId(ps state.PipelineState) string {
+	return fmt.Sprintf("val_%v", ps.StateKey())
+}
+
+// UserStateCoderId returns the key coder id of a user state

Review Comment:
   Thanks! Hopefully that saves a follow up linting PR 😅 



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] riteshghorse commented on a diff in pull request #22897: Add map state in the Go Sdk

Posted by GitBox <gi...@apache.org>.
riteshghorse commented on code in PR #22897:
URL: https://github.com/apache/beam/pull/22897#discussion_r956313322


##########
sdks/go/pkg/beam/core/runtime/graphx/translate.go:
##########
@@ -1376,3 +1398,13 @@ func UpdateDefaultEnvWorkerType(typeUrn string, pyld []byte, p *pipepb.Pipeline)
 	}
 	return errors.Errorf("unable to find dependency with %q role in environment with ID %q,", URNArtifactGoWorkerRole, defaultEnvId)
 }
+
+// UserStateCoderId returns the coder id of a user state
+func UserStateCoderId(ps state.PipelineState) string {
+	return fmt.Sprintf("val_%v", ps.StateKey())
+}
+
+// UserStateCoderId returns the key coder id of a user state

Review Comment:
   ```suggestion
   // UserStateKeyCoderId returns the key coder id of a user state
   ```



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] riteshghorse commented on a diff in pull request #22897: Add map state in the Go Sdk

Posted by GitBox <gi...@apache.org>.
riteshghorse commented on code in PR #22897:
URL: https://github.com/apache/beam/pull/22897#discussion_r956240694


##########
sdks/go/pkg/beam/core/runtime/exec/userstate.go:
##########
@@ -106,12 +110,12 @@ func (s *stateProvider) WriteValueState(val state.Transaction) error {
 	return nil
 }
 
-// ReadBagState reads a ReadBagState state from the State API
+// ReadBagState reads a BagState state from the State API

Review Comment:
   ```suggestion
   // ReadBagState reads a bag state from the State API
   ```



##########
sdks/go/pkg/beam/core/runtime/exec/userstate.go:
##########
@@ -162,6 +165,105 @@ func (s *stateProvider) WriteBagState(val state.Transaction) error {
 	return nil
 }
 
+// ReadMapStateValue reads a value from the map state given a key.

Review Comment:
   ```suggestion
   // ReadMapStateValue reads a value from the map state for a given map key.
   ```



##########
sdks/go/pkg/beam/core/state/state_test.go:
##########
@@ -116,6 +117,50 @@ func (s *fakeProvider) ExtractOutputFn(userStateID string) reflectx.Func {
 	return nil
 }
 
+// ReadMapStateValue(userStateID string, key interface{}) (interface{}, []Transaction, error)

Review Comment:
   Did you mean to add doc comments for the functions below?



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] damccorm commented on pull request #22897: Add map state in the Go Sdk

Posted by GitBox <gi...@apache.org>.
damccorm commented on PR #22897:
URL: https://github.com/apache/beam/pull/22897#issuecomment-1227730403

   R: @riteshghorse 
   
   FWIW, this is a larger PR because I thought it would make more sense as a cohesive change and because its so similar to the other changes. With that said, as you review I'd probably recommend thinking about it in 3 pieces (and reviewing it in that order):
   
   1. Extending existing plumbing to support keyed state (both translate.go's)
   2. Execution side stuff (statemgr.go + userstate.go)
   3. User side stuff (state.go + fn.go).
   4. Misc interface + tests (everything else)
   
   I'd probably recommend reading the PR in that order.
   
   Generally, the reason this PR is a little bigger is because we're using new protos for multimap state instead of the ones we've been using for bag state.


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] damccorm merged pull request #22897: Add map state in the Go Sdk

Posted by GitBox <gi...@apache.org>.
damccorm merged PR #22897:
URL: https://github.com/apache/beam/pull/22897


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] codecov[bot] commented on pull request #22897: Add map state in the Go Sdk

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #22897:
URL: https://github.com/apache/beam/pull/22897#issuecomment-1227737455

   # [Codecov](https://codecov.io/gh/apache/beam/pull/22897?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#22897](https://codecov.io/gh/apache/beam/pull/22897?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (7eb9f89) into [master](https://codecov.io/gh/apache/beam/commit/a58f3d2c89fc88166d3f4af7c2d9b4c463d7718a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a58f3d2) will **decrease** coverage by `0.20%`.
   > The diff coverage is `18.44%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #22897      +/-   ##
   ==========================================
   - Coverage   73.87%   73.67%   -0.21%     
   ==========================================
     Files         713      713              
     Lines       94318    94676     +358     
   ==========================================
   + Hits        69682    69751      +69     
   - Misses      23348    23635     +287     
   - Partials     1288     1290       +2     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | go | `50.78% <18.44%> (-0.41%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/22897?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [sdks/go/pkg/beam/core/graph/fn.go](https://codecov.io/gh/apache/beam/pull/22897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL2dyYXBoL2ZuLmdv) | `84.54% <0.00%> (ø)` | |
   | [sdks/go/pkg/beam/core/runtime/exec/data.go](https://codecov.io/gh/apache/beam/pull/22897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy9kYXRhLmdv) | `0.00% <ø> (ø)` | |
   | [sdks/go/pkg/beam/core/runtime/exec/translate.go](https://codecov.io/gh/apache/beam/pull/22897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy90cmFuc2xhdGUuZ28=) | `12.94% <0.00%> (-0.27%)` | :arrow_down: |
   | [sdks/go/pkg/beam/core/runtime/graphx/translate.go](https://codecov.io/gh/apache/beam/pull/22897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZ3JhcGh4L3RyYW5zbGF0ZS5nbw==) | `38.91% <0.00%> (-0.92%)` | :arrow_down: |
   | [sdks/go/pkg/beam/core/runtime/harness/statemgr.go](https://codecov.io/gh/apache/beam/pull/22897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvaGFybmVzcy9zdGF0ZW1nci5nbw==) | `37.75% <0.00%> (-8.85%)` | :arrow_down: |
   | [sdks/go/pkg/beam/pardo.go](https://codecov.io/gh/apache/beam/pull/22897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9wYXJkby5nbw==) | `42.10% <0.00%> (-2.34%)` | :arrow_down: |
   | [sdks/go/pkg/beam/core/runtime/exec/userstate.go](https://codecov.io/gh/apache/beam/pull/22897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy91c2Vyc3RhdGUuZ28=) | `6.43% <1.30%> (-4.62%)` | :arrow_down: |
   | [sdks/go/pkg/beam/core/state/state.go](https://codecov.io/gh/apache/beam/pull/22897/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3N0YXRlL3N0YXRlLmdv) | `75.28% <75.82%> (+0.28%)` | :arrow_up: |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] damccorm commented on a diff in pull request #22897: Add map state in the Go Sdk

Posted by GitBox <gi...@apache.org>.
damccorm commented on code in PR #22897:
URL: https://github.com/apache/beam/pull/22897#discussion_r956288147


##########
sdks/go/pkg/beam/core/state/state_test.go:
##########
@@ -116,6 +117,50 @@ func (s *fakeProvider) ExtractOutputFn(userStateID string) reflectx.Func {
 	return nil
 }
 
+// ReadMapStateValue(userStateID string, key interface{}) (interface{}, []Transaction, error)

Review Comment:
   Whoops, no this is just a leftover bad copy paste as I moved the definitions over. I shouldn't need doc comments for test functions, but I did remove these. Good catch!



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org