lostluck commented on code in PR #33711:
URL: https://github.com/apache/beam/pull/33711#discussion_r1929945495
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -94,3 +97,152 @@ func TestExpansionAddr(t *testing.T) {
t.Errorf("The function that ExpansionAddr(%v) returned did not
work correctly. For the expansionAddr field in options, got %v, want %v",
test.addr, o.expansionAddr, test.addr)
}
}
+
+// TestOutputType tests the OutputType option for setting the output type
+// in an SQL transformation in Beam. It verifies both cases: when
+// components are provided and when they are not. The test checks if
+// the 'outType' field in the options is set correctly based on the
+// output type and components.
+func TestOutputType(t *testing.T) {
+ testCases := []struct {
+ name string
+ typ reflect.Type
+ components []typex.FullType
+ wantNil bool
+ }{
+ {
+ name: "output_type_without_components",
+ typ: reflect.TypeOf(int64(0)),
+ },
+ {
+ name: "output_type_with_components",
+ typ: reflect.TypeOf(int64(0)),
+ components:
[]typex.FullType{typex.New(reflect.TypeOf(""))},
+ },
+ }
+
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ o := &options{}
+
+ var opt Option
+ if len(tc.components) > 0 {
+ opt = OutputType(tc.typ, tc.components...)
+ } else {
+ opt = OutputType(tc.typ)
+ }
+
+ opt(o)
+
+ var expected typex.FullType
+ if len(tc.components) > 0 {
+ expected = typex.New(tc.typ, tc.components...)
+ } else {
+ expected = typex.New(tc.typ)
+ }
+
+ opts := cmp.Options{
+ cmp.Comparer(func(x, y typex.FullType) bool {
+ // Compare only the type and components
+ return x.Type() == y.Type()
+ }),
+ }
+ if !cmp.Equal(o.outType, expected, opts) {
+ t.Errorf("OutputType() failed:\n%s",
cmp.Diff(expected, o.outType, opts))
+ }
Review Comment:
conventionally, since Diff reports "" when there is no difference, there's
no need to do the check twice.
Also, when printing a diff, it's useful to provide the reading guidance. In
this case (-want, +got) which indicates what was missing from what we received
compared to the expected value, but also what was extra.
```suggestion
if d := cmp.Diff(expected, o.outType, opts); d != "" {
t.Errorf("OutputType() failed: (-want,
+got)\n%s", d)
}
```
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -94,3 +97,152 @@ func TestExpansionAddr(t *testing.T) {
t.Errorf("The function that ExpansionAddr(%v) returned did not
work correctly. For the expansionAddr field in options, got %v, want %v",
test.addr, o.expansionAddr, test.addr)
}
}
+
+// TestOutputType tests the OutputType option for setting the output type
+// in an SQL transformation in Beam. It verifies both cases: when
+// components are provided and when they are not. The test checks if
+// the 'outType' field in the options is set correctly based on the
+// output type and components.
+func TestOutputType(t *testing.T) {
+ testCases := []struct {
+ name string
+ typ reflect.Type
+ components []typex.FullType
+ wantNil bool
+ }{
+ {
+ name: "output_type_without_components",
+ typ: reflect.TypeOf(int64(0)),
+ },
+ {
+ name: "output_type_with_components",
+ typ: reflect.TypeOf(int64(0)),
+ components:
[]typex.FullType{typex.New(reflect.TypeOf(""))},
+ },
+ }
+
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ o := &options{}
+
+ var opt Option
+ if len(tc.components) > 0 {
+ opt = OutputType(tc.typ, tc.components...)
+ } else {
+ opt = OutputType(tc.typ)
+ }
+
+ opt(o)
+
+ var expected typex.FullType
+ if len(tc.components) > 0 {
+ expected = typex.New(tc.typ, tc.components...)
+ } else {
+ expected = typex.New(tc.typ)
+ }
+
+ opts := cmp.Options{
+ cmp.Comparer(func(x, y typex.FullType) bool {
+ // Compare only the type and components
+ return x.Type() == y.Type()
+ }),
+ }
+ if !cmp.Equal(o.outType, expected, opts) {
+ t.Errorf("OutputType() failed:\n%s",
cmp.Diff(expected, o.outType, opts))
+ }
+ })
+ }
+}
+
+// TestTransform tests the behavior of the Transform function
+// in the context of SQL transformations. It checks that the function
+// panics when an output type is missing.
+func TestTransform_MissingOutputType(t *testing.T) {
+ p := beam.NewPipeline()
+ s := p.Root()
+ col := beam.Create(s, 1, 2, 3)
+
+ defer func() {
+ r := recover()
+ if r == nil {
+ t.Error("Transform() with missing output type should
panic")
+ }
+ if msg, ok := r.(string); !ok || msg != "output type must be
specified for sql.Transform" {
+ t.Errorf("Transform() unexpected panic message: %v", r)
+ }
+ }()
+
+ Transform(s, "SELECT value FROM test", Input("test", col))
+}
+
+// TestMultipleOptions tests applying multiple options at once
+// and verifying that they are all correctly applied to the options object.
+func TestMultipleOptions(t *testing.T) {
+ p := beam.NewPipeline()
+ s := p.Root()
+ col := beam.Create(s, 1, 2, 3)
+
+ testCases := []struct {
+ name string
+ inputName string
+ dialect string
+ expansionAddr string
+ typ reflect.Type
+ customOpt sqlx.Option
+ }{
+ {
+ name: "all_options",
+ inputName: "test",
+ dialect: "zetasql",
+ expansionAddr: "localhost:8080",
+ typ: reflect.TypeOf(int64(0)),
+ customOpt: sqlx.Option{Urn: "test"},
+ },
+ }
+
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ o := &options{
+ inputs: make(map[string]beam.PCollection),
+ }
+
+ opts := []Option{
+ Input(tc.inputName, col),
+ Dialect(tc.dialect),
+ ExpansionAddr(tc.expansionAddr),
+ OutputType(tc.typ),
+ }
+
+ // Apply all options
+ for _, opt := range opts {
+ opt(o)
+ }
+ o.Add(tc.customOpt)
+
+ // Verify all fields
+ if got, ok := o.inputs[tc.inputName]; !ok {
+ t.Errorf("Input option with name %q not applied
correctly: got nothing, want PCollection", tc.inputName)
+ } else if !reflect.DeepEqual(got, col) {
Review Comment:
Same comment as before about using cmp instead of reflect.DeepEqual.
Here and the usage below.
In a code review, it's common for reviewers to *not* need to enumerate every
line where the same mistake is repeated, as we expect contributors to apply the
comment throughout the PR, where it would apply.
If there's a potential reason not to apply the comment where it might apply,
it is good to 1) ask if it should apply there and 2) clarify why you think it
shouldn't apply.
--------
In this case, you could consolidate several of these checks into one by
instead directly constructing the desired end state of the options, and then
using a single cmp.Diff to handle detecting mismatched fields. Please repeat
the form I provided in the earlier comment.
##########
.gitignore:
##########
@@ -154,3 +154,7 @@ playground/cloudfunction.zip
# Ignore .test-infra/metrics/github_runs_prefetcher/code.zip
# as its generated with terraform
.test-infra/metrics/sync/github/github_runs_prefetcher/code.zip
+
+# Ignore ./sdks/go/pkg/beam/transforms/sql/coverage.out
+# as it is generated by running tests
+sdks/go/pkg/beam/transforms/sql/coverage.out
Review Comment:
I would either not include such a specific ignore clause, or make it apply
to all coverage.out files (as we do for others)
In this case, it's an uncommon file that won't be present in most case.
Infrequency means we shouldn't ignore it, so please remove the changes from
gitignore.
##########
sdks/go/pkg/beam/transforms/sql/sql_test.go:
##########
@@ -94,3 +97,152 @@ func TestExpansionAddr(t *testing.T) {
t.Errorf("The function that ExpansionAddr(%v) returned did not
work correctly. For the expansionAddr field in options, got %v, want %v",
test.addr, o.expansionAddr, test.addr)
}
}
+
+// TestOutputType tests the OutputType option for setting the output type
+// in an SQL transformation in Beam. It verifies both cases: when
+// components are provided and when they are not. The test checks if
+// the 'outType' field in the options is set correctly based on the
+// output type and components.
+func TestOutputType(t *testing.T) {
+ testCases := []struct {
+ name string
+ typ reflect.Type
+ components []typex.FullType
+ wantNil bool
+ }{
+ {
+ name: "output_type_without_components",
+ typ: reflect.TypeOf(int64(0)),
+ },
+ {
+ name: "output_type_with_components",
+ typ: reflect.TypeOf(int64(0)),
+ components:
[]typex.FullType{typex.New(reflect.TypeOf(""))},
+ },
+ }
+
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ o := &options{}
+
+ var opt Option
+ if len(tc.components) > 0 {
+ opt = OutputType(tc.typ, tc.components...)
+ } else {
+ opt = OutputType(tc.typ)
+ }
+
+ opt(o)
+
+ var expected typex.FullType
+ if len(tc.components) > 0 {
+ expected = typex.New(tc.typ, tc.components...)
+ } else {
+ expected = typex.New(tc.typ)
+ }
+
+ opts := cmp.Options{
+ cmp.Comparer(func(x, y typex.FullType) bool {
+ // Compare only the type and components
+ return x.Type() == y.Type()
+ }),
+ }
+ if !cmp.Equal(o.outType, expected, opts) {
+ t.Errorf("OutputType() failed:\n%s",
cmp.Diff(expected, o.outType, opts))
+ }
+ })
+ }
+}
+
+// TestTransform tests the behavior of the Transform function
+// in the context of SQL transformations. It checks that the function
+// panics when an output type is missing.
+func TestTransform_MissingOutputType(t *testing.T) {
+ p := beam.NewPipeline()
+ s := p.Root()
+ col := beam.Create(s, 1, 2, 3)
+
+ defer func() {
+ r := recover()
+ if r == nil {
+ t.Error("Transform() with missing output type should
panic")
+ }
+ if msg, ok := r.(string); !ok || msg != "output type must be
specified for sql.Transform" {
+ t.Errorf("Transform() unexpected panic message: %v", r)
+ }
+ }()
+
+ Transform(s, "SELECT value FROM test", Input("test", col))
+}
+
+// TestMultipleOptions tests applying multiple options at once
+// and verifying that they are all correctly applied to the options object.
+func TestMultipleOptions(t *testing.T) {
+ p := beam.NewPipeline()
+ s := p.Root()
+ col := beam.Create(s, 1, 2, 3)
Review Comment:
While this portion is common to every test case, it could cause problems to
keep re-using them. In particular, each case would be added all to the same
pipeline. Please just put them to be created anew at the top of the test.Run
function in the test case loop.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
