[GitHub] [beam] lostluck commented on a change in pull request #13188: [BEAM-11108] Add a version of TextIO implemented via SDF.

[GitBox <gi...@apache.org>]

lostluck commented on a change in pull request #13188:
URL: https://github.com/apache/beam/pull/13188#discussion_r512208575



##########
File path: sdks/go/pkg/beam/io/textio/sdf_test.go
##########
@@ -0,0 +1,63 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements.  See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License.  You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package textio
+
+import (
+	"context"
+	"testing"
+)
+
+// TestReadSdf tests that readSdf successfully reads a test text file, and
+// outputs the correct number of lines for it, even for an exceedingly long
+// line.
+func TestReadSdf(t *testing.T) {
+	f := "../../../../data/textio_test.txt"
+	f, size, err := sizeFn(context.Background(), f)
+	if err != nil {
+		t.Fatalf("sizing failed: %v", err)
+	}
+
+	lines := fakeReadSdfFn(t, f, size)
+	want := 1
+	if len(lines) != 1 {
+		t.Fatalf("received %v lines, want %v", len(lines), want)
+	}
+}
+
+// fakeReadSdfFn calls the methods in readSdfFn on a single input to simulate
+// executing an SDF, and outputs all elements produced by that input.
+func fakeReadSdfFn(t *testing.T, f string, size int64) []string {

Review comment:
       Notionally this kind of helper is a reason to have "real" SDF actuation on the Go Direct Runner, so testing the IOs is as simple as running a go direct pipeline. 
   That would be a good candidate for a separate JIRA on the topic as a subtask of  BEAM-11076 if it doesn't already work.

##########
File path: sdks/go/pkg/beam/io/rtrackers/offsetrange/offsetrange.go
##########
@@ -89,6 +89,28 @@ func (r Restriction) EvenSplits(num int64) (splits []Restriction) {
 	return splits
 }
 
+// SizedSplits splits a restriction into multiple restrictions of the given
+// size. If the restriction cannot be evenly split, the final restriction will
+// be the remainder.
+//
+// Example: (0, 24) split into size 10s -> {(0, 10), (10, 20), (20, 24)}
+//
+// Size should be greater than 0. Otherwise there is no way to split the
+// restriction and this function will return the original restriction.
+func (r Restriction) SizedSplits(size int64) (splits []Restriction) {

Review comment:
       Consider adding a unit test, with the stated example, a "real world" example (largely with larger numbers involved, like the 64MB chunks used later in this PR), and edge cases, like exact splits, and off by 1 errors (both sides) etc (to validate that we don't somehow end up with 0 sized splits.)




----------------------------------------------------------------
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.

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