henryken commented on a change in pull request #11564: URL: https://github.com/apache/beam/pull/11564#discussion_r418049904
########## File path: learning/katas/go/Core Transforms/Map/ParDo/test/task_test.go ########## @@ -0,0 +1,49 @@ +// 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 test + +import ( + "github.com/apache/beam/sdks/go/pkg/beam" + "github.com/apache/beam/sdks/go/pkg/beam/testing/passert" + "github.com/apache/beam/sdks/go/pkg/beam/testing/ptest" + "pardo/pkg/task" + "testing" +) + +func TestApplyTransform(t *testing.T) { + p, s := beam.NewPipelineWithRoot() + tests := []struct { + input beam.PCollection + want []interface{} + }{ + { + input: beam.Create(s, -1, -2, -3, -4, -5), + want: []interface{}{-10, -20, -30, -40, -50}, + }, + { + input: beam.Create(s, 1, 2, 3, 4, 5), + want: []interface{}{10, 20, 30, 40, 50}, + }, + } + for _, tt := range tests { + got := task.ApplyTransform(s, tt.input) + passert.Equals(s, got, tt.want...) + if err := ptest.Run(p); err != nil { + t.Errorf("ApplyTransform(\"%v\") = %v", tt.input, err) Review comment: I'd prefer to just use `t.Error(err)`. The current formatting prints out something that is not intuitive? e.g. ApplyTransform("{2: int/int[varintz] GLO}") = panic: value -300 present, but not expected ########## File path: learning/katas/go/Core Transforms/Map/ParDo/test/task_test.go ########## @@ -0,0 +1,49 @@ +// 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 test + +import ( + "github.com/apache/beam/sdks/go/pkg/beam" + "github.com/apache/beam/sdks/go/pkg/beam/testing/passert" + "github.com/apache/beam/sdks/go/pkg/beam/testing/ptest" + "pardo/pkg/task" + "testing" +) + +func TestApplyTransform(t *testing.T) { + p, s := beam.NewPipelineWithRoot() + tests := []struct { + input beam.PCollection + want []interface{} + }{ + { + input: beam.Create(s, -1, -2, -3, -4, -5), Review comment: I think one input is probably sufficient. Both inputs are not fundamentally different. ########## File path: learning/katas/go/Core Transforms/Map/ParDo/cmd/main.go ########## @@ -0,0 +1,41 @@ +// 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 main + +import ( + "context" + "github.com/apache/beam/sdks/go/pkg/beam" + "github.com/apache/beam/sdks/go/pkg/beam/log" + "github.com/apache/beam/sdks/go/pkg/beam/x/beamx" + "github.com/apache/beam/sdks/go/pkg/beam/x/debug" + "pardo/pkg/task" +) + +func main() { + ctx := context.Background() + + p, s := beam.NewPipelineWithRoot() + + col := task.ApplyTransform(s, beam.Create(s, 1, 2, 3, 4, 5)) Review comment: I would prefer to separate the Create into its own line to make it clear as a pipeline. And can we rename 'col' to 'output'? ``` numbers := beam.Create(s, 1, 2, 3, 4, 5) output := task.ApplyTransform(s, numbers) ``` ########## File path: learning/katas/go/Core Transforms/Map/ParDo/pkg/task/task.go ########## @@ -0,0 +1,25 @@ +// 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 task + +import "github.com/apache/beam/sdks/go/pkg/beam" + +func ApplyTransform(s beam.Scope, input beam.PCollection) beam.PCollection { + processFn := func(element int) int { Review comment: Can we rename the function to 'multiplyBy10Fn"? ########## File path: learning/katas/go/Core Transforms/Map/ParDo/pkg/task/task.go ########## @@ -0,0 +1,25 @@ +// 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 task + +import "github.com/apache/beam/sdks/go/pkg/beam" + +func ApplyTransform(s beam.Scope, input beam.PCollection) beam.PCollection { + processFn := func(element int) int { Review comment: I see another lesson using module function instead of local variable. Shall we extract it as a module function for consistency? ########## File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/test/task_test.go ########## @@ -0,0 +1,48 @@ +// 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 test + +import ( + "github.com/apache/beam/sdks/go/pkg/beam" + "github.com/apache/beam/sdks/go/pkg/beam/testing/passert" + "github.com/apache/beam/sdks/go/pkg/beam/testing/ptest" + "pardo_onetomany/pkg/task" + "testing" +) + +func TestTask(t *testing.T) { + p, s := beam.NewPipelineWithRoot() + tests := []struct { + input beam.PCollection + want []interface{} + }{ + { + input: beam.Create(s, "Hello Beam", "It is awesome"), Review comment: I think one input is probably sufficient. Both inputs are not fundamentally different. ########## File path: learning/katas/go/Core Transforms/Map/MapElements/test/task_test.go ########## @@ -0,0 +1,48 @@ +// 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 test + +import ( + "github.com/apache/beam/sdks/go/pkg/beam" + "github.com/apache/beam/sdks/go/pkg/beam/testing/passert" + "github.com/apache/beam/sdks/go/pkg/beam/testing/ptest" + "mapelements/pkg/task" + "testing" +) + +func TestTask(t *testing.T) { + p, s := beam.NewPipelineWithRoot() + tests := []struct { + input beam.PCollection + want []interface{} + }{ + { + input: beam.Create(s, -1, -2, -3, -4, -5), Review comment: I think one input is probably sufficient. Both inputs are not fundamentally different. ########## File path: learning/katas/go/Core Transforms/Map/MapElements/cmd/main.go ########## @@ -0,0 +1,39 @@ +// 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 main + +import ( + "context" + "github.com/apache/beam/sdks/go/pkg/beam" + "github.com/apache/beam/sdks/go/pkg/beam/log" + "github.com/apache/beam/sdks/go/pkg/beam/x/beamx" + "github.com/apache/beam/sdks/go/pkg/beam/x/debug" + "mapelements/pkg/task" +) + +func main() { + p, s := beam.NewPipelineWithRoot() + + col := task.ApplyTransform(s, beam.Create(s, 1, 2, 3, 4, 5)) Review comment: I would prefer to separate the Create into its own line to make it clear as a pipeline. And can we rename 'col' to 'output'? ``` numbers := beam.Create(s, 1, 2, 3, 4, 5) output := task.ApplyTransform(s, numbers) ``` ########## File path: learning/katas/go/Core Transforms/Map/MapElements/test/task_test.go ########## @@ -0,0 +1,48 @@ +// 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 test + +import ( + "github.com/apache/beam/sdks/go/pkg/beam" + "github.com/apache/beam/sdks/go/pkg/beam/testing/passert" + "github.com/apache/beam/sdks/go/pkg/beam/testing/ptest" + "mapelements/pkg/task" + "testing" +) + +func TestTask(t *testing.T) { + p, s := beam.NewPipelineWithRoot() + tests := []struct { + input beam.PCollection + want []interface{} + }{ + { + input: beam.Create(s, -1, -2, -3, -4, -5), + want: []interface{}{-5, -10, -15, -20, -25}, + }, + { + input: beam.Create(s, 1, 2, 3, 4, 5), + want: []interface{}{5, 10, 15, 20, 25}, + }, + } + for _, tt := range tests { + got := task.ApplyTransform(s, tt.input) + passert.Equals(s, got, tt.want...) + if err := ptest.Run(p); err != nil { + t.Errorf("ApplyTransform(\"%v\") = %v", tt.input, err) Review comment: I'd prefer to just use t.Error(err). The current formatting prints out something that is not intuitive? e.g. ApplyTransform("{2: int/int[varintz] GLO}") = panic: value -300 present, but not expected ########## File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/pkg/task/task.go ########## @@ -0,0 +1,33 @@ +// 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 task + +import ( + "github.com/apache/beam/sdks/go/pkg/beam" + "strings" +) + +func ApplyTransform(s beam.Scope, input beam.PCollection) beam.PCollection { + return beam.ParDo(s, processFn, input) +} + +func processFn(input string, emit func(out string)) { Review comment: Can we rename the function to 'tokenizeFn"? ########## File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/test/task_test.go ########## @@ -0,0 +1,48 @@ +// 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 test + +import ( + "github.com/apache/beam/sdks/go/pkg/beam" + "github.com/apache/beam/sdks/go/pkg/beam/testing/passert" + "github.com/apache/beam/sdks/go/pkg/beam/testing/ptest" + "pardo_onetomany/pkg/task" + "testing" +) + +func TestTask(t *testing.T) { + p, s := beam.NewPipelineWithRoot() + tests := []struct { + input beam.PCollection + want []interface{} + }{ + { + input: beam.Create(s, "Hello Beam", "It is awesome"), + want: []interface{}{"Hello", "Beam", "It", "is", "awesome"}, + }, + { + input: beam.Create(s, "Hello Beam. It is awesome."), + want: []interface{}{"Hello", "Beam.", "It", "is", "awesome."}, + }, + } + for _, tt := range tests { + got := task.ApplyTransform(s, tt.input) + passert.Equals(s, got, tt.want...) + if err := ptest.Run(p); err != nil { + t.Errorf("ApplyTransform(\"%v\") = %v", tt.input, err) Review comment: I'd prefer to just use t.Error(err). The current formatting prints out something that is not intuitive? e.g. ApplyTransform("{2: string/string[string] GLO}") = panic: value Itx present, but not expected ########## File path: learning/katas/go/Core Transforms/Map/MapElements/pkg/task/task.go ########## @@ -0,0 +1,33 @@ +// 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 task + +import "github.com/apache/beam/sdks/go/pkg/beam" + +func ApplyTransform(s beam.Scope, input beam.PCollection) beam.PCollection { + processFn := &multiplyByFn{ Review comment: Would rather have this inline similar to the example given. https://github.com/apache/beam/blob/master/sdks/go/examples/contains/contains.go#L48 ########## File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/cmd/main.go ########## @@ -0,0 +1,39 @@ +// 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 main + +import ( + "context" + "github.com/apache/beam/sdks/go/pkg/beam" + "github.com/apache/beam/sdks/go/pkg/beam/log" + "github.com/apache/beam/sdks/go/pkg/beam/x/beamx" + "github.com/apache/beam/sdks/go/pkg/beam/x/debug" + "pardo_onetomany/pkg/task" +) + +func main() { + p, s := beam.NewPipelineWithRoot() + + col := task.ApplyTransform(s, beam.Create(s, "Hello Beam", "It is awesome")) Review comment: I would prefer to separate the Create into its own line to make it clear as a pipeline. And can we rename 'col' to 'output'? ``` sentences:= beam.Create(s, "Hello Beam", "It is awesome") output := task.ApplyTransform(s, sentences) ``` ########## File path: learning/katas/go/Core Transforms/Map/MapElements/pkg/task/task.go ########## @@ -0,0 +1,33 @@ +// 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 task + +import "github.com/apache/beam/sdks/go/pkg/beam" + +func ApplyTransform(s beam.Scope, input beam.PCollection) beam.PCollection { + processFn := &multiplyByFn{ + Factor: 5, + } + return beam.ParDo(s, processFn, input) +} + +type multiplyByFn struct { Review comment: I'd suggest having the empty struct revealed for the students. They can then fill in the struct body. ########## File path: learning/katas/go/Core Transforms/Map/MapElements/task.md ########## @@ -0,0 +1,34 @@ +<!-- + 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. +--> +# Mapping Elements using structs Review comment: I'd prefer to name this lesson "ParDo Struct". Since there is no MapElements construct in Go, we don't need to mimic the Java version. ########## File path: learning/katas/go/Core Transforms/Map/MapElements/pkg/task/task.go ########## @@ -0,0 +1,33 @@ +// 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 task + +import "github.com/apache/beam/sdks/go/pkg/beam" + +func ApplyTransform(s beam.Scope, input beam.PCollection) beam.PCollection { + processFn := &multiplyByFn{ + Factor: 5, + } + return beam.ParDo(s, processFn, input) +} + +type multiplyByFn struct { + Factor int +} + +func (f *multiplyByFn) ProcessElement(input int) int { Review comment: I'd suggest having the empty ProcessElement function revealed for the students. They can then fill in the function body. ########## File path: learning/katas/go/Core Transforms/Map/FlatMapElements/task.md ########## @@ -0,0 +1,31 @@ +<!-- + 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. +--> +In this kata, we continue the use of a DoFn struct to map a single element into multiple elements Review comment: I think we don't need this task. This is essentially the same as "ParDo Struct" and it's also not a FlatMap. ---------------------------------------------------------------- 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: [email protected]
