[YAML] Add JSON parsing to PubSub IO. #28754
Conversation
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control |
Codecov Report
@@ Coverage Diff @@
## master #28754 +/- ##
==========================================
- Coverage 72.21% 72.20% -0.02%
==========================================
Files 684 685 +1
Lines 101241 101338 +97
==========================================
+ Hits 73116 73171 +55
- Misses 26546 26588 +42
Partials 1579 1579
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 19 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
R: @liferoad |
| # limitations under the License. | ||
| # | ||
|
|
||
| """Utilities for converting between JSON and Beam Schema'd data. |
There was a problem hiding this comment.
Do we expect these functions will be used internally? If so, can we document this as the internal utilities and no backward compatibility guaranteed.
There was a problem hiding this comment.
These may be fine to expose, but added a disclaimer just in case.
| raise ValueError(f"Unrecognized type_info: {type_info!r}") | ||
|
|
||
|
|
||
| def json_parser(beam_schema: schema_pb2.Schema) -> Callable[[bytes], beam.Row]: |
There was a problem hiding this comment.
looks like only json_parser and json_formatter are used by other modules. I think it would be better to document these clearly. Docstrings will be good at least for these two functions.
There was a problem hiding this comment.
Added basic docstrings to all of these methods. Hopefully the name and signatures should make them clear as well.
| b'{"generator": {"x": 0, "y": 0}, "rank": 1}', | ||
| {'weierstrass': 'y^2+y=x^3-x', 'label': '37a'}) | ||
| ])): | ||
| result = p | YamlTransform( |
There was a problem hiding this comment.
can we have some error case tests here to improve the coverage?
There was a problem hiding this comment.
Done. This actually uncovered a bug when transforms are explicitly named in some cases. Thanks.
|
LGTM. Just some minor comments. |
This makes #28694 more useful.
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.