[
https://issues.apache.org/jira/browse/BEAM-14489?focusedWorklogId=772804&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-772804
]
ASF GitHub Bot logged work on BEAM-14489:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 20/May/22 12:26
Start Date: 20/May/22 12:26
Worklog Time Spent: 10m
Work Description: damccorm commented on code in PR #17712:
URL: https://github.com/apache/beam/pull/17712#discussion_r878087209
##########
sdks/go/pkg/beam/io/textio/sdf.go:
##########
@@ -37,17 +37,21 @@ func init() {
// ReadSdf is a variation of Read implemented via SplittableDoFn. This should
// result in increased performance with runners that support splitting.
+//
+// Deprecated: Called directly by Read, use that instead.
func ReadSdf(s beam.Scope, glob string) beam.PCollection {
- s = s.Scope("textio.ReadSdf")
+ s = s.Scope("textio.Read")
Review 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)
##########
sdks/go/pkg/beam/io/textio/sdf.go:
##########
@@ -37,17 +37,21 @@ func init() {
// ReadSdf is a variation of Read implemented via SplittableDoFn. This should
// result in increased performance with runners that support splitting.
+//
+// Deprecated: Called directly by Read, use that instead.
func ReadSdf(s beam.Scope, glob string) beam.PCollection {
- s = s.Scope("textio.ReadSdf")
+ s = s.Scope("textio.Read")
filesystem.ValidateScheme(glob)
return readSdf(s, beam.Create(s, glob))
}
// ReadAllSdf is a variation of ReadAll implemented via SplittableDoFn. This
// should result in increased performance with runners that support splitting.
+//
+// Deprecated: Called directly by ReadAll, use that instead.
func ReadAllSdf(s beam.Scope, col beam.PCollection) beam.PCollection {
- s = s.Scope("textio.ReadAllSdf")
+ s = s.Scope("textio.ReadAll")
Review 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
Issue Time Tracking
-------------------
Worklog Id: (was: 772804)
Time Spent: 40m (was: 0.5h)
> Make all TextIO SDF.
> --------------------
>
> Key: BEAM-14489
> URL: https://issues.apache.org/jira/browse/BEAM-14489
> Project: Beam
> Issue Type: Improvement
> Components: sdk-go
> Reporter: Robert Burke
> Assignee: Robert Burke
> Priority: P2
> Time Spent: 40m
> Remaining Estimate: 0h
>
> We currently have two kinds of textio.Read an SDF version and a Non SDF
> version. They should be unified (removing the non-SDF one) to avoid confusing
> users.
> Since there are likely a number of various packages using one or the other,
> we will point others to the existing Read calls but mark the SDF ones as
> deprecated. They will not be removed, as that's the Go way.
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
