You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/04/29 04:28:52 UTC
[GitHub] [beam] damondouglas opened a new pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
damondouglas opened a new pull request #11564:
URL: https://github.com/apache/beam/pull/11564
This pull request adds a section related to Core Transforms with one Map transforms lesson to the Go SDK katas. It patterns after the existing [Java katas](https://github.com/apache/beam/tree/master/learning/katas/java/Core%20Transforms/Map):
- ParDo
- ParDo OneToMany
- MapElements
- FlatMapElements
I would like to request @henryken and @lostluck to review this pull request. Thank you for allowing me to be part of this project.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [x] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Apex | Dataflow | Flink | Gearpump | Samza | Spark
--- | --- | --- | --- | --- | --- | --- | ---
Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Gearpump/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://builds.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
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
[GitHub] [beam] damondouglas commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r419141964
##########
File path: learning/katas/go/Core Transforms/Map/ParDo/pkg/task/task.go
##########
@@ -18,8 +18,9 @@ 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 {
- return element * 10
- }
- return beam.ParDo(s, processFn, input)
+ return beam.ParDo(s, multiplyBy10Fn, input)
}
+
+func multiplyBy10Fn(element int) int {
+ return element * 10
Review comment:
What's odd is that I saw the following when testing the course in both IntelliJ and GoLand:
<img width="647" alt="Screen Shot 2020-05-03 at 11 25 01 AM" src="https://user-images.githubusercontent.com/762456/80922441-092c4980-8d32-11ea-9796-a8d0e0ede524.png">
----------------------------------------------------------------
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
[GitHub] [beam] damondouglas commented on pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on pull request #11564:
URL: https://github.com/apache/beam/pull/11564#issuecomment-623165292
Run RAT Precommit
----------------------------------------------------------------
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
[GitHub] [beam] damondouglas commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r419141964
##########
File path: learning/katas/go/Core Transforms/Map/ParDo/pkg/task/task.go
##########
@@ -18,8 +18,9 @@ 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 {
- return element * 10
- }
- return beam.ParDo(s, processFn, input)
+ return beam.ParDo(s, multiplyBy10Fn, input)
}
+
+func multiplyBy10Fn(element int) int {
+ return element * 10
Review comment:
What's odd is that I saw the following when testing the course in both IntelliJ and Goland:
<img width="647" alt="Screen Shot 2020-05-03 at 11 25 01 AM" src="https://user-images.githubusercontent.com/762456/80922441-092c4980-8d32-11ea-9796-a8d0e0ede524.png">
----------------------------------------------------------------
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
[GitHub] [beam] damondouglas commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r420451192
##########
File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/task.md
##########
@@ -0,0 +1,32 @@
+<!--
+ 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.
+-->
+
+# ParDo - One to Many
+
+In the previous kata we learned that ParDo maps a single element into another element.
+In this kata we will map a single element into many by splitting a sentence into words.
+
+**Kata:** Please write a ParDo that maps each input sentence into words tokenized by whitespace (" ").
+
+<div class="hint">
+ Use <a href="https://godoc.org/github.com/apache/beam/sdks/go/pkg/beam#ParDo">
+ ParDo</a>
Review comment:
@lostluck Do you mean this? If so, I could check across the various tasks of the existing katas to make sure its consistent.
```
<div class="hint">
Use <a href="https://godoc.org/github.com/apache/beam/sdks/go/pkg/beam#ParDo">
beam.ParDo</a>
```
----------------------------------------------------------------
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
[GitHub] [beam] henryken commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
henryken commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r419240777
##########
File path: learning/katas/go/Core Transforms/Map/ParDo/pkg/task/task.go
##########
@@ -18,8 +18,9 @@ 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 {
- return element * 10
- }
- return beam.ParDo(s, processFn, input)
+ return beam.ParDo(s, multiplyBy10Fn, input)
}
+
+func multiplyBy10Fn(element int) int {
+ return element * 10
Review comment:
This looks good now
----------------------------------------------------------------
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
[GitHub] [beam] damondouglas commented on pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on pull request #11564:
URL: https://github.com/apache/beam/pull/11564#issuecomment-623621585
> Retest this please
Is this addressed to me? Would you like me to retest something?
----------------------------------------------------------------
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
[GitHub] [beam] damondouglas edited a comment on pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
damondouglas edited a comment on pull request #11564:
URL: https://github.com/apache/beam/pull/11564#issuecomment-623621585
> Retest this please
Is this addressed to me? Would you like me to retest something?
That's "addressed" to the jenkins bot to retest the PR via whatever automated selection of tests it requires. Since the bot is particular, I needed to separate it from my other comment so it would work.
----------------------------------------------------------------
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
[GitHub] [beam] lostluck commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
lostluck commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r419627705
##########
File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/task.md
##########
@@ -0,0 +1,32 @@
+<!--
+ 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.
+-->
+
+# ParDo - One to Many
+
+In the previous kata we learned that ParDo maps a single element into another element.
+In this kata we will map a single element into many by splitting a sentence into words.
+
+**Kata:** Please write a ParDo that maps each input sentence into words tokenized by whitespace (" ").
+
+<div class="hint">
+ Use <a href="https://godoc.org/github.com/apache/beam/sdks/go/pkg/beam#ParDo">
+ ParDo</a>
+ with <a href="https://godoc.org/github.com/apache/beam/sdks/go/pkg/beam#hdr-DoFns">
Review comment:
DoFns are generally a specific thing, so an article is handy.
```suggestion
with a <a href="https://godoc.org/github.com/apache/beam/sdks/go/pkg/beam#hdr-DoFns">
```
##########
File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/task.md
##########
@@ -0,0 +1,32 @@
+<!--
+ 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.
+-->
+
+# ParDo - One to Many
+
+In the previous kata we learned that ParDo maps a single element into another element.
+In this kata we will map a single element into many by splitting a sentence into words.
+
+**Kata:** Please write a ParDo that maps each input sentence into words tokenized by whitespace (" ").
+
+<div class="hint">
+ Use <a href="https://godoc.org/github.com/apache/beam/sdks/go/pkg/beam#ParDo">
+ ParDo</a>
Review comment:
Consider using the qualifed import name, like they'd see in their Go code (eg. beam.ParDo) , rather than just the single method. It would look odd to other languages, but the generally explicit package/provenance of identifiers is a hallmark of Go.
##########
File path: learning/katas/go/Core Transforms/Map/ParDo/task.md
##########
@@ -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.
+-->
+
+# ParDo
+
+ParDo is a Beam transform for generic parallel processing. The ParDo processing paradigm is similar to the “Map”
+phase of a Map/Shuffle/Reduce-style algorithm: a ParDo transform considers each element in the input PCollection,
+performs some processing function (your user code) on that element, and emits zero, one, or multiple elements to an
+output PCollection.
+
+**Kata:** Please write a simple ParDo that maps the input element by multiplying it by 10.
Review comment:
A possible adjacent task/step is converting a func DoFn into a Structural DoFn. Yes, this is relatively simple, but from a learning standpoint, it makes the distinction pretty clear, while not asking users too much else that might conflate with it. (eg. Getting the wrong idea that funcs must be 1:1 vs 1:many/none, vs structs etc).
Not necessary to do it in this PR.
----------------------------------------------------------------
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
[GitHub] [beam] henryken commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
henryken commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r419807983
##########
File path: learning/katas/go/Core Transforms/Map/ParDo struct/pkg/task/task.go
##########
@@ -18,10 +18,7 @@ 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)
+ return beam.ParDo(s, &multiplyByFn{Factor: 5}, input)
Review comment:
This looks good now! Thanks @damondouglas!
----------------------------------------------------------------
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
[GitHub] [beam] lostluck commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
lostluck commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r421830719
##########
File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/task.md
##########
@@ -0,0 +1,32 @@
+<!--
+ 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.
+-->
+
+# ParDo - One to Many
+
+In the previous kata we learned that ParDo maps a single element into another element.
+In this kata we will map a single element into many by splitting a sentence into words.
+
+**Kata:** Please write a ParDo that maps each input sentence into words tokenized by whitespace (" ").
+
+<div class="hint">
+ Use <a href="https://godoc.org/github.com/apache/beam/sdks/go/pkg/beam#ParDo">
+ ParDo</a>
Review comment:
I do!
----------------------------------------------------------------
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
[GitHub] [beam] damondouglas commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r418386969
##########
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:
May we consider keeping it? I found [these guidelines](https://github.com/golang/go/wiki/CodeReviewComments#variable-names) on variable naming. I wasn't sure if they apply but my intuition is that `multiplyBy10Fn` might be too long? I think even my `processFn` is too long. I actually prefer to keep in line [with the example from the godocs](https://godoc.org/github.com/apache/beam/sdks/go/pkg/beam#ParDo) but working with the IntelliJ answer placeholder made the presentation of a separate function unclear.
----------------------------------------------------------------
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
[GitHub] [beam] henryken commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
henryken commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r419240682
##########
File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/pkg/task/task.go
##########
@@ -21,10 +21,10 @@ import (
)
func ApplyTransform(s beam.Scope, input beam.PCollection) beam.PCollection {
- return beam.ParDo(s, processFn, input)
+ return beam.ParDo(s, tokenizeFn, input)
}
-func processFn(input string, emit func(out string)) {
+func tokenizeFn(input string, emit func(out string)) {
Review comment:
This looks good now
----------------------------------------------------------------
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
[GitHub] [beam] damondouglas commented on pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on pull request #11564:
URL: https://github.com/apache/beam/pull/11564#issuecomment-622222881
----------------------------------------------------------------
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
[GitHub] [beam] lostluck commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
lostluck commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r421831198
##########
File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/task.md
##########
@@ -0,0 +1,32 @@
+<!--
+ 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.
+-->
+
+# ParDo - One to Many
+
+In the previous kata we learned that ParDo maps a single element into another element.
+In this kata we will map a single element into many by splitting a sentence into words.
+
+**Kata:** Please write a ParDo that maps each input sentence into words tokenized by whitespace (" ").
+
+<div class="hint">
+ Use <a href="https://godoc.org/github.com/apache/beam/sdks/go/pkg/beam#ParDo">
+ ParDo</a>
Review comment:
Rather, I do. But I can merge this in as is and the change can happen in another PR or it can happen this PR. Whichever you prefer.
Sorry for the delay in getting back to this.
----------------------------------------------------------------
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
[GitHub] [beam] henryken commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
henryken commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r419240949
##########
File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/pkg/task/task.go
##########
@@ -21,10 +21,10 @@ import (
)
func ApplyTransform(s beam.Scope, input beam.PCollection) beam.PCollection {
- return beam.ParDo(s, processFn, input)
+ return beam.ParDo(s, tokenizeFn, input)
}
-func processFn(input string, emit func(out string)) {
+func tokenizeFn(input string, emit func(out string)) {
Review comment:
This looks good now
##########
File path: learning/katas/go/Core Transforms/Map/ParDo struct/pkg/task/task.go
##########
@@ -18,10 +18,7 @@ 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)
+ return beam.ParDo(s, &multiplyByFn{Factor: 5}, input)
Review comment:
This seems to have an issue now. There is no placeholder at all. And the task description doesn't load properly.
Could it be because the directory name needs to be renamed to "ParDo Struct" with capital S?
----------------------------------------------------------------
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
[GitHub] [beam] henryken commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [beam] henryken commented on pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
henryken commented on pull request #11564:
URL: https://github.com/apache/beam/pull/11564#issuecomment-623783144
Please wait for the course uploading before merging this PR.
----------------------------------------------------------------
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
[GitHub] [beam] damondouglas commented on pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on pull request #11564:
URL: https://github.com/apache/beam/pull/11564#issuecomment-623491943
Thank you @henryken for your patience. I found the course preview function in IntelliJ Edu inconsistent between GoLand and IntelliJ. In my regular testing using GoLand, I load by first closing the project and reopening via navigation to the original folder. If I open the project through any other means in GoLand, when activating the course preview, it seems to pull from a cached source. I haven't completely figured out the issue with GoLand. IntelliJ seems to work as expected.
I deleted the entire `ParDo struct` and recreated it to `ParDo Struct` as well as removed my entire `src` and `pkg` folder in my `$GOHOME`. I tested the course preview in both GoLand and IntelliJ and it masks the code using the placeholders where expected.
----------------------------------------------------------------
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
[GitHub] [beam] henryken commented on pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
henryken commented on pull request #11564:
URL: https://github.com/apache/beam/pull/11564#issuecomment-623808517
Awesome! This PR can be merged now. Thanks @damondouglas!
----------------------------------------------------------------
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
[GitHub] [beam] damondouglas commented on pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on pull request #11564:
URL: https://github.com/apache/beam/pull/11564#issuecomment-623799315
@henryken and @lostluck I updated [the Stepik course](https://stepik.org/course/70387) and commited the `*-remote-info.yaml` files. Thank you both for your help.
----------------------------------------------------------------
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
[GitHub] [beam] damondouglas commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r419142387
##########
File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/pkg/task/task.go
##########
@@ -21,10 +21,10 @@ import (
)
func ApplyTransform(s beam.Scope, input beam.PCollection) beam.PCollection {
- return beam.ParDo(s, processFn, input)
+ return beam.ParDo(s, tokenizeFn, input)
}
-func processFn(input string, emit func(out string)) {
+func tokenizeFn(input string, emit func(out string)) {
Review comment:
Similar to the previous placeholder issue. This is what appears when I tested the course in both IntelliJ and GoLand:
<img width="717" alt="Screen Shot 2020-05-03 at 11 18 41 AM" src="https://user-images.githubusercontent.com/762456/80922546-bacb7a80-8d32-11ea-9486-524d57333235.png">
----------------------------------------------------------------
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
[GitHub] [beam] damondouglas commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r418386969
##########
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:
May we consider keeping it? I found [these guidelines](https://github.com/golang/go/wiki/CodeReviewComments#variable-names) on variable naming. I wasn't sure if they apply but my intuition is that `multiplyBy10Fn` might be too long? I think even my `processFn` is too long. I actually prefer to keep in line [with the example from the godocs](https://godoc.org/github.com/apache/beam/sdks/go/pkg/beam#ParDo) but working with the IntelliJ answer placeholder made the presentation of a separate function unclear.
----------------------------------------------------------------
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
[GitHub] [beam] henryken commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
henryken commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r419807137
##########
File path: learning/katas/go/Core Transforms/Map/ParDo/task.md
##########
@@ -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.
+-->
+
+# ParDo
+
+ParDo is a Beam transform for generic parallel processing. The ParDo processing paradigm is similar to the “Map”
+phase of a Map/Shuffle/Reduce-style algorithm: a ParDo transform considers each element in the input PCollection,
+performs some processing function (your user code) on that element, and emits zero, one, or multiple elements to an
+output PCollection.
+
+**Kata:** Please write a simple ParDo that maps the input element by multiplying it by 10.
Review comment:
@lostluck, the space will not create any problem. It is friendlier for the learner to see the lesson and task names in the natural way versus using underscore name.
----------------------------------------------------------------
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
[GitHub] [beam] damondouglas commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r421834698
##########
File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/task.md
##########
@@ -0,0 +1,32 @@
+<!--
+ 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.
+-->
+
+# ParDo - One to Many
+
+In the previous kata we learned that ParDo maps a single element into another element.
+In this kata we will map a single element into many by splitting a sentence into words.
+
+**Kata:** Please write a ParDo that maps each input sentence into words tokenized by whitespace (" ").
+
+<div class="hint">
+ Use <a href="https://godoc.org/github.com/apache/beam/sdks/go/pkg/beam#ParDo">
+ ParDo</a>
Review comment:
I created [BEAM-9928](https://issues.apache.org/jira/browse/BEAM-9928) so this is not forgotten. My vote is to merge the request.
----------------------------------------------------------------
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
[GitHub] [beam] lostluck commented on pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
lostluck commented on pull request #11564:
URL: https://github.com/apache/beam/pull/11564#issuecomment-625528370
Retest this please
----------------------------------------------------------------
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
[GitHub] [beam] henryken commented on a change in pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
henryken commented on a change in pull request #11564:
URL: https://github.com/apache/beam/pull/11564#discussion_r418443043
##########
File path: learning/katas/go/Core Transforms/Map/ParDo/pkg/task/task.go
##########
@@ -18,8 +18,9 @@ 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 {
- return element * 10
- }
- return beam.ParDo(s, processFn, input)
+ return beam.ParDo(s, multiplyBy10Fn, input)
}
+
+func multiplyBy10Fn(element int) int {
+ return element * 10
Review comment:
The "* 10" is not covered by the answer placeholder. Is it intentional?
##########
File path: learning/katas/go/Core Transforms/Map/lesson-info.yaml
##########
@@ -20,5 +20,4 @@
content:
- ParDo
- ParDo OneToMany
-- MapElements
-- FlatMapElements
+- ParDo struct
Review comment:
Can maybe slightly rename to "ParDo Struct"?
##########
File path: learning/katas/go/Core Transforms/Map/ParDo struct/pkg/task/task.go
##########
@@ -18,10 +18,7 @@ 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)
+ return beam.ParDo(s, &multiplyByFn{Factor: 5}, input)
Review comment:
How about having the whole "&multiplyByFn{...}" be part of the answer placeholder?
##########
File path: learning/katas/go/Core Transforms/Map/ParDo struct/task.md
##########
@@ -16,10 +16,10 @@
specific language governing permissions and limitations
under the License.
-->
-# Mapping Elements using structs
+# Using a struct as a DoFn
Review comment:
Can we have the ParDo mentioned, e.g. "ParDo - using a struct as a DoFn"?
##########
File path: learning/katas/go/Core Transforms/Map/ParDo OneToMany/pkg/task/task.go
##########
@@ -21,10 +21,10 @@ import (
)
func ApplyTransform(s beam.Scope, input beam.PCollection) beam.PCollection {
- return beam.ParDo(s, processFn, input)
+ return beam.ParDo(s, tokenizeFn, input)
}
-func processFn(input string, emit func(out string)) {
+func tokenizeFn(input string, emit func(out string)) {
Review comment:
The answer placeholder(s) seems weird here. Maybe it requires a fix?
Or alternatively, we can use a similar answer placeholder as in the "ParDo" task, i.e. having just empty "fund ApplyTransforms"?
----------------------------------------------------------------
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
[GitHub] [beam] lostluck commented on pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
lostluck commented on pull request #11564:
URL: https://github.com/apache/beam/pull/11564#issuecomment-621426498
----------------------------------------------------------------
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
[GitHub] [beam] lostluck commented on pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
lostluck commented on pull request #11564:
URL: https://github.com/apache/beam/pull/11564#issuecomment-623617273
Retest this please
----------------------------------------------------------------
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
[GitHub] [beam] lostluck commented on pull request #11564: [Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas
Posted by GitBox <gi...@apache.org>.
lostluck commented on pull request #11564:
URL: https://github.com/apache/beam/pull/11564#issuecomment-623617721
(sadly, only comitters can trigger the tests even after the first time. Your commands were correct)
I'm doing this review now. Thank you for your patience!
----------------------------------------------------------------
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