Add StringSet metrics to Python SDK #31969
Conversation
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
R: @robertwb |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
| elif len(y) == 0: | ||
| return x | ||
| else: | ||
| return set.union(x, y) |
There was a problem hiding this comment.
wondering if it makes sense to use return set.union(x, y) unconditionally. Could there be any concerns about side effects due to Aggregation returning a mutable collection that can be modified elsewhere after aggregation was computed?
There was a problem hiding this comment.
this mirrors #31803 . I wonder in most cases the aggregation encounters one empty and one non-empty cells, so a short cut optimization is reasonable here.
Could there be any concerns about side effects due to Aggregation returning a mutable collection
This is a good point. I was hesitating if I should return set / frozenset . However in Python frozenset isn't instance of set (isinstance(frozenset(), set) is False) while Java ImmutableSet is Set, I ended up with set. Do you think we should make the result of StringSet query a frozenset instance everywhere?
There was a problem hiding this comment.
I think it depends on the lifecycle of these sets; given Java implementation, my guess is that it is probably not a concern.
|
PreCommit YAML Xlang Direct fails with slf4jlogger classDefNotFoundError. Test passed on an earlier commit and the change later on are trivial naming changes. Likely bad gradle cache and merging for now |
* Add StringSet metrics to Python SDK * Address comments * Use string_set everywhere * fix leftover SET_STRING_TYPE -> STRING_SET_TYPE
Please add a meaningful description for your change here
Direct runner and Flink runner (loopback) working. For Dataflow runner, needs to regenerate apiclient, will be done in a separate PR
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.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.