[BEAM-14489] Remove non-SDF version of TextIO. #17712
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17712 +/- ##
=======================================
Coverage 74.00% 74.01%
=======================================
Files 695 695
Lines 91798 91798
=======================================
+ Hits 67938 67944 +6
+ Misses 22612 22608 -4
+ Partials 1248 1246 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Assigning reviewers. If you would like to opt out of this review, comment R: @jrmccluskey for label go. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
sdks/go/pkg/beam/io/textio/sdf.go
Outdated
| func ReadAllSdf(s beam.Scope, col beam.PCollection) beam.PCollection { | ||
| s = s.Scope("textio.ReadAllSdf") | ||
| s = s.Scope("textio.ReadAll") |
There was a problem hiding this comment.
Thoughts on moving this file's contents into the main textio.go file? If we're removing the distinction between read and readSdf, splitting doesn't make sense anymore IMO
There was a problem hiding this comment.
I agree, no reason to logically split it out if it's SDFs all the way down
There was a problem hiding this comment.
Agreed. Great call! Moved and ensured placement was reasonable, and dropped vestigial SDFs that were no longer necessary (though added documentation that they are SDFs and that they're useful for splitting within files.)
sdks/go/pkg/beam/io/textio/sdf.go
Outdated
| func ReadSdf(s beam.Scope, glob string) beam.PCollection { | ||
| s = s.Scope("textio.ReadSdf") | ||
| s = s.Scope("textio.Read") |
There was a problem hiding this comment.
Might be overthinking this, but is this (minorly) breaking if anyone is using this for a composite transform or checking it in a test? It might be worth pulling out the rest of this function out into its own helper and then having each caller of ReadSdf set its scope before calling it.
Relatedly, does this overwrite the scope set by ReadAllSdf? (probably not worth changing at this point for the same breaking reason, I'm just curious)
There was a problem hiding this comment.
IIRC it becomes a sub-scope.
There was a problem hiding this comment.
Agreed it could be confusing, or breaking, so changed it to maintain scopes.
sdks/go/pkg/beam/io/textio/sdf.go
Outdated
| func ReadSdf(s beam.Scope, glob string) beam.PCollection { | ||
| s = s.Scope("textio.ReadSdf") | ||
| s = s.Scope("textio.Read") |
There was a problem hiding this comment.
Agreed it could be confusing, or breaking, so changed it to maintain scopes.
sdks/go/pkg/beam/io/textio/sdf.go
Outdated
| func ReadAllSdf(s beam.Scope, col beam.PCollection) beam.PCollection { | ||
| s = s.Scope("textio.ReadAllSdf") | ||
| s = s.Scope("textio.ReadAll") |
There was a problem hiding this comment.
Agreed. Great call! Moved and ensured placement was reasonable, and dropped vestigial SDFs that were no longer necessary (though added documentation that they are SDFs and that they're useful for splitting within files.)
Removes the non-SDF version of TextIO, so that it's old pattern can't be copied. All meaningful runners understand SDFs, and we're confident in their execution at this stage.
Per Go policy, the old *Sdf methods are marked deprecated, but will not be removed until a major version change, because this is a user use package.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.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.