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/06/10 22:00:01 UTC
[GitHub] [beam] codeBehindMe opened a new pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
codeBehindMe opened a new pull request #11976:
URL: https://github.com/apache/beam/pull/11976
Previously, when a mismatch of the dimensions emitted by the DoFn and the dimensions emitted by the ParDo was encountered, a less than helpful message of `expected 1 output. Found: []` was returned. This caused some confusion especially when paired with ParDo and ParDo0 which may be counterintuitive arrangement to most.
In this patch, we provide a more helpful error message which correctly reference the DoFn which has the misalignment and the ParDo which the misalignment of dimensions were captured by. It also recommends a ParDo function which may be used by the DoFn in question.
We introduce two new functions to the pardo package as well as a new function to the reflectx package to help us construct this error message.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [ ] 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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r438710063
##########
File path: sdks/go/pkg/beam/core/util/reflectx/functions.go
##########
@@ -45,3 +45,13 @@ func LoadFunction(ptr uintptr, t reflect.Type) interface{} {
*(*unsafe.Pointer)(unsafe.Pointer(v.Addr().Pointer())) = unsafe.Pointer(p)
return v.Interface()
}
+
+//FunctionOutputSize returns the dimensions of the output of a function.
+// Panics if the type is not a function.
+func FunctionOutputSize(fn interface{}) int {
Review comment:
Hmm I see, perhaps I've gone down a wrong path here then, let me have another review of the code and see what existing tooling is there to resolve 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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r439729323
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
+ doFnName := reflectx.FunctionName(doFn)
Review comment:
@lostluck , do we cast it from interface{} to graph.Fn type?
----------------------------------------------------------------
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] codeBehindMe commented on pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on pull request #11976:
URL: https://github.com/apache/beam/pull/11976#issuecomment-645222759
Awesome! Exciting. Thanks, i'll take on another ticket from the backlog. Cheers, Aaron
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440534339
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +412,36 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(formatParDoError(dofn, len(ret), 7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//formatParDoError is a helper function to provide a more concise error
Review comment:
7c612c9db2
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r439726844
##########
File path: sdks/go/pkg/beam/core/util/reflectx/functions_test.go
##########
@@ -41,3 +45,19 @@ func TestLoadFunction(t *testing.T) {
t.Errorf("got %d, wanted %d", out[0].Int(), testFunction())
}
}
+
+func TestFunctionOutputSize(t *testing.T) {
+ expected := 1
+ received := FunctionOutputSize(testFunction)
+ if received != expected {
+ t.Errorf("got %d, wanted %d", received, expected)
+ }
+}
+
+func TestFunction2OutputSize(t *testing.T) {
+ expected := 2
+ received := FunctionOutputSize(testFunction2)
Review comment:
a4d5083
----------------------------------------------------------------
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] codeBehindMe commented on pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on pull request #11976:
URL: https://github.com/apache/beam/pull/11976#issuecomment-642290142
@damondouglas Hey mate, could you give this a quick once over.
----------------------------------------------------------------
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] codeBehindMe commented on pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on pull request #11976:
URL: https://github.com/apache/beam/pull/11976#issuecomment-644073690
Address current comments, waiting for feedback.
----------------------------------------------------------------
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 #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
lostluck commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r441157202
##########
File path: sdks/go/pkg/beam/pardo_test.go
##########
@@ -0,0 +1,54 @@
+// 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 beam
+
+import (
+ "testing"
+)
+
+func TestRecommendParDo(t *testing.T) {
+ var tests = []struct {
+ name string
+ outputDim int
+ want string
+ }{
+ {"zero outputs", 0, "ParDo0"},
+ {"one output", 1, "ParDo"},
+ {"more than 7 outputs", 10, "ParDoN"},
+ }
+
+ for _, tt := range tests {
+ testName := tt.name
+ t.Run(testName, func(t *testing.T) {
+ got := recommendParDo(tt.outputDim)
+ if got != tt.want {
+ t.Errorf("RecommendParDo(%v) = %v, want %v", tt.outputDim, got, tt.want)
+ }
+ })
+ }
+}
+
+func testFunction() int64 {
+ return 42
+}
+
+func TestFormatParDoError(t *testing.T) {
+ got := formatParDoError(testFunction, 2, 1)
Review comment:
Please do keep the test! My comment was that because the logic that's unique to formatParDoError is so small, the test can also be small.
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440107978
##########
File path: sdks/go/pkg/beam/pardo_test.go
##########
@@ -0,0 +1,40 @@
+// 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 beam
+
+import "testing"
+
+func TestRecommendParDo(t *testing.T) {
+ var tests = []struct {
+ name string
+ outputDim int
+ want string
+ }{
+ {"zero outputs", 0, "ParDo0"},
+ {"one output", 1, "ParDo"},
+ {"more than 7 outputs", 10, "ParDoN"},
+ }
+
+ for _, tt := range tests {
+ testName := tt.name
+ t.Run(testName, func(t *testing.T) {
+ got := recommendParDo(tt.outputDim)
+ if got != tt.want {
+ t.Errorf("RecommendParDo(%v) = %v, want %v", tt.outputDim, got, tt.want)
Review comment:
db99bf446ac38a68291983c222755cdaea0f1de3
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r439726617
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
+ doFnName := reflectx.FunctionName(doFn)
+ doFnOutSize := reflectx.FunctionOutputSize(doFn)
+
+ parDoName := reflectx.FunctionName(parDo)
+ parDoOutSize := reflectx.FunctionOutputSize(parDo)
+
+ useParDo := reflectx.FunctionName(RecommendParDo(doFnOutSize))
+ return fmt.Sprintf("DoFn %v has %v outptus, but %v requires %v outputs, Use %v instead.", doFnName, doFnOutSize, parDoName, parDoOutSize, useParDo)
+
+}
+
+// recommendParDo takes a in a DoFns emit dimension and recommends the correct
+// ParDo to use.
+func RecommendParDo(emitDim int) interface{} {
Review comment:
db99bf446a
----------------------------------------------------------------
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] codeBehindMe commented on pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on pull request #11976:
URL: https://github.com/apache/beam/pull/11976#issuecomment-642290717
@damondouglas If you could give me some feedback that'll be nice.
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440531674
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +412,36 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(formatParDoError(dofn, len(ret), 7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//formatParDoError is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+//
+// We construct a new graph.Fn using the doFn which is passed. We explicitly
+// ignore the error since we already know that its already a DoFn type as
+// TryParDo would have panicked otherwise.
+func formatParDoError(doFn interface{}, emitSize int, parDoSize int) string {
+ doFun, _ := graph.NewFn(doFn)
+ doFnName := doFun.Name()
+
+ thisParDo := recommendParDo(parDoSize) // Conveniently keeps the API slim.
Review comment:
I like brevity, i will implement 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] lostluck commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
lostluck commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440321837
##########
File path: sdks/go/pkg/beam/pardo_test.go
##########
@@ -0,0 +1,54 @@
+// 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 beam
+
+import (
+ "testing"
+)
+
+func TestRecommendParDo(t *testing.T) {
+ var tests = []struct {
+ name string
+ outputDim int
+ want string
+ }{
+ {"zero outputs", 0, "ParDo0"},
+ {"one output", 1, "ParDo"},
+ {"more than 7 outputs", 10, "ParDoN"},
+ }
+
+ for _, tt := range tests {
+ testName := tt.name
+ t.Run(testName, func(t *testing.T) {
+ got := recommendParDo(tt.outputDim)
+ if got != tt.want {
+ t.Errorf("RecommendParDo(%v) = %v, want %v", tt.outputDim, got, tt.want)
+ }
+ })
+ }
+}
+
+func testFunction() int64 {
+ return 42
+}
+
+func TestFormatParDoError(t *testing.T) {
+ got := formatParDoError(testFunction, 2, 1)
Review comment:
Since the "complicated" bit for this is in the recommendParDo code, which is already being tested very well, this test should be sufficient for the parts in formatParDoError. I don't expect the formatting to change any time soon.
We do have the english grammar nit ("1 outputs"), but handling that grammar correction isn't worth the extra code to resolve it.
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +412,36 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(formatParDoError(dofn, len(ret), 7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//formatParDoError is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+//
+// We construct a new graph.Fn using the doFn which is passed. We explicitly
+// ignore the error since we already know that its already a DoFn type as
+// TryParDo would have panicked otherwise.
+func formatParDoError(doFn interface{}, emitSize int, parDoSize int) string {
+ doFun, _ := graph.NewFn(doFn)
+ doFnName := doFun.Name()
+
+ thisParDo := recommendParDo(parDoSize) // Conveniently keeps the API slim.
Review comment:
Ah clever! But not in the hard to debug way. :D I like it.
Optional Nit: It's no longer really recommending, but naming the appropriate DoFn for the size in this use. parDoForSize might be a more accurate name, but, honestly, either is fine.
Had there only been the one call, I'd have suggested inlining it since the switch isn't that large.
Either way, since it's being used twice, that justifies that switch being in it's own function.
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +412,36 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(formatParDoError(dofn, len(ret), 7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//formatParDoError is a helper function to provide a more concise error
Review comment:
Nit: add a space between // and formatParDoError
##########
File path: sdks/go/pkg/beam/pardo_test.go
##########
@@ -0,0 +1,54 @@
+// 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 beam
+
+import (
+ "testing"
+)
+
+func TestRecommendParDo(t *testing.T) {
+ var tests = []struct {
+ name string
+ outputDim int
+ want string
+ }{
+ {"zero outputs", 0, "ParDo0"},
+ {"one output", 1, "ParDo"},
+ {"more than 7 outputs", 10, "ParDoN"},
+ }
+
+ for _, tt := range tests {
+ testName := tt.name
+ t.Run(testName, func(t *testing.T) {
+ got := recommendParDo(tt.outputDim)
+ if got != tt.want {
+ t.Errorf("RecommendParDo(%v) = %v, want %v", tt.outputDim, got, tt.want)
+ }
+ })
+ }
+}
+
+func testFunction() int64 {
Review comment:
It might be worth commenting here that this function is just to validate that the name is sourced properly in TestFormatParDoError since the number of errors don't match.
##########
File path: sdks/go/pkg/beam/pardo_test.go
##########
@@ -0,0 +1,54 @@
+// 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 beam
+
+import (
+ "testing"
+)
+
+func TestRecommendParDo(t *testing.T) {
+ var tests = []struct {
+ name string
+ outputDim int
+ want string
+ }{
+ {"zero outputs", 0, "ParDo0"},
+ {"one output", 1, "ParDo"},
Review comment:
Please add 3 more test cases here for 2 and 7,8 to cover those boundary conditions.
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r439728573
##########
File path: sdks/go/pkg/beam/core/util/reflectx/functions_test.go
##########
@@ -41,3 +45,19 @@ func TestLoadFunction(t *testing.T) {
t.Errorf("got %d, wanted %d", out[0].Int(), testFunction())
}
}
+
+func TestFunctionOutputSize(t *testing.T) {
+ expected := 1
+ received := FunctionOutputSize(testFunction)
+ if received != expected {
+ t.Errorf("got %d, wanted %d", received, expected)
+ }
+}
+
+func TestFunction2OutputSize(t *testing.T) {
+ expected := 2
+ received := FunctionOutputSize(testFunction2)
+ if received != expected {
+ t.Errorf("got %d, wanted %d", received, expected)
Review comment:
945dc7a264
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440107551
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
+ doFnName := reflectx.FunctionName(doFn)
+ doFnOutSize := reflectx.FunctionOutputSize(doFn)
+
+ parDoName := reflectx.FunctionName(parDo)
+ parDoOutSize := reflectx.FunctionOutputSize(parDo)
+
+ useParDo := reflectx.FunctionName(RecommendParDo(doFnOutSize))
+ return fmt.Sprintf("DoFn %v has %v outptus, but %v requires %v outputs, Use %v instead.", doFnName, doFnOutSize, parDoName, parDoOutSize, useParDo)
Review comment:
45f4a84e57
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -428,31 +428,20 @@ func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
parDoName := reflectx.FunctionName(parDo)
parDoOutSize := reflectx.FunctionOutputSize(parDo)
- useParDo := reflectx.FunctionName(RecommendParDo(doFnOutSize))
+ useParDo := reflectx.FunctionName(recommendParDo(doFnOutSize))
Review comment:
45f4a84e57
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440108500
##########
File path: sdks/go/pkg/beam/core/util/reflectx/functions.go
##########
@@ -45,3 +45,13 @@ func LoadFunction(ptr uintptr, t reflect.Type) interface{} {
*(*unsafe.Pointer)(unsafe.Pointer(v.Addr().Pointer())) = unsafe.Pointer(p)
return v.Interface()
}
+
+//FunctionOutputSize returns the dimensions of the output of a function.
+// Panics if the type is not a function.
+func FunctionOutputSize(fn interface{}) int {
Review comment:
Removed
----------------------------------------------------------------
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 merged pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
lostluck merged pull request #11976:
URL: https://github.com/apache/beam/pull/11976
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r439729323
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
+ doFnName := reflectx.FunctionName(doFn)
Review comment:
@lostluck , do we cast it from interface{} to graph.Fn type? Is that safe to do so?
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440110392
##########
File path: sdks/go/pkg/beam/core/util/reflectx/functions.go
##########
@@ -45,3 +45,13 @@ func LoadFunction(ptr uintptr, t reflect.Type) interface{} {
*(*unsafe.Pointer)(unsafe.Pointer(v.Addr().Pointer())) = unsafe.Pointer(p)
return v.Interface()
}
+
+//FunctionOutputSize returns the dimensions of the output of a function.
+// Panics if the type is not a function.
+func FunctionOutputSize(fn interface{}) int {
Review comment:
8c9e030351
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440531674
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +412,36 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(formatParDoError(dofn, len(ret), 7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//formatParDoError is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+//
+// We construct a new graph.Fn using the doFn which is passed. We explicitly
+// ignore the error since we already know that its already a DoFn type as
+// TryParDo would have panicked otherwise.
+func formatParDoError(doFn interface{}, emitSize int, parDoSize int) string {
+ doFun, _ := graph.NewFn(doFn)
+ doFnName := doFun.Name()
+
+ thisParDo := recommendParDo(parDoSize) // Conveniently keeps the API slim.
Review comment:
I like brevity, I will implement 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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440532539
##########
File path: sdks/go/pkg/beam/pardo_test.go
##########
@@ -0,0 +1,54 @@
+// 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 beam
+
+import (
+ "testing"
+)
+
+func TestRecommendParDo(t *testing.T) {
+ var tests = []struct {
+ name string
+ outputDim int
+ want string
+ }{
+ {"zero outputs", 0, "ParDo0"},
+ {"one output", 1, "ParDo"},
+ {"more than 7 outputs", 10, "ParDoN"},
+ }
+
+ for _, tt := range tests {
+ testName := tt.name
+ t.Run(testName, func(t *testing.T) {
+ got := recommendParDo(tt.outputDim)
+ if got != tt.want {
+ t.Errorf("RecommendParDo(%v) = %v, want %v", tt.outputDim, got, tt.want)
+ }
+ })
+ }
+}
+
+func testFunction() int64 {
+ return 42
+}
+
+func TestFormatParDoError(t *testing.T) {
+ got := formatParDoError(testFunction, 2, 1)
Review comment:
I'm open to spend a bit more time to do this, or would you like me to remove the test here?
I agree, covering the the `recommendParDo` function is more important. Happy to defer to your decision here.
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440106666
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
+ doFnName := reflectx.FunctionName(doFn)
Review comment:
45f4a84e57
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440729833
##########
File path: sdks/go/pkg/beam/pardo_test.go
##########
@@ -0,0 +1,54 @@
+// 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 beam
+
+import (
+ "testing"
+)
+
+func TestRecommendParDo(t *testing.T) {
+ var tests = []struct {
+ name string
+ outputDim int
+ want string
+ }{
+ {"zero outputs", 0, "ParDo0"},
+ {"one output", 1, "ParDo"},
Review comment:
d9b5ef08e9
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440531818
##########
File path: sdks/go/pkg/beam/pardo_test.go
##########
@@ -0,0 +1,54 @@
+// 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 beam
+
+import (
+ "testing"
+)
+
+func TestRecommendParDo(t *testing.T) {
+ var tests = []struct {
+ name string
+ outputDim int
+ want string
+ }{
+ {"zero outputs", 0, "ParDo0"},
+ {"one output", 1, "ParDo"},
+ {"more than 7 outputs", 10, "ParDoN"},
+ }
+
+ for _, tt := range tests {
+ testName := tt.name
+ t.Run(testName, func(t *testing.T) {
+ got := recommendParDo(tt.outputDim)
+ if got != tt.want {
+ t.Errorf("RecommendParDo(%v) = %v, want %v", tt.outputDim, got, tt.want)
+ }
+ })
+ }
+}
+
+func testFunction() int64 {
Review comment:
Good idea, I will implement this. Thanks.
----------------------------------------------------------------
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 #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
lostluck commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r438475207
##########
File path: sdks/go/pkg/beam/core/util/reflectx/functions_test.go
##########
@@ -41,3 +45,19 @@ func TestLoadFunction(t *testing.T) {
t.Errorf("got %d, wanted %d", out[0].Int(), testFunction())
}
}
+
+func TestFunctionOutputSize(t *testing.T) {
+ expected := 1
+ received := FunctionOutputSize(testFunction)
+ if received != expected {
+ t.Errorf("got %d, wanted %d", received, expected)
+ }
+}
+
+func TestFunction2OutputSize(t *testing.T) {
Review comment:
Conventionally, if there are multiple cases for a given test, they should be combined into the same function, and run as a loop.
See https://gobyexample.com/testing for an excellent example.
Otherwise, this function should be named TestFunctionOutputSize_2
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
+ doFnName := reflectx.FunctionName(doFn)
+ doFnOutSize := reflectx.FunctionOutputSize(doFn)
+
+ parDoName := reflectx.FunctionName(parDo)
+ parDoOutSize := reflectx.FunctionOutputSize(parDo)
+
+ useParDo := reflectx.FunctionName(RecommendParDo(doFnOutSize))
+ return fmt.Sprintf("DoFn %v has %v outptus, but %v requires %v outputs, Use %v instead.", doFnName, doFnOutSize, parDoName, parDoOutSize, useParDo)
+
+}
+
+// recommendParDo takes a in a DoFns emit dimension and recommends the correct
+// ParDo to use.
+func RecommendParDo(emitDim int) interface{} {
+ switch {
+ case emitDim == 0:
+ return ParDo0
+ case emitDim == 1:
+ return ParDo
+ case emitDim == 2:
+ return ParDo2
+ case emitDim == 3:
+ return ParDo3
+ case emitDim == 4:
+ return ParDo4
+ case emitDim == 5:
+ return ParDo5
+ case emitDim == 6:
+ return ParDo6
+ case emitDim == 7:
+ return ParDo7
+ }
+ return ParDoN
+}
Review comment:
What I'd recommend here instead though is to return a string instead of the function.
```
switch emitDim {
case 0,2,3,4,5,6,7:
return fmt.Sprintf("ParDo%d", emitDim)
case 1:
return "ParDo"
default:
return "ParDoN"
}
```
Easier to read, and see if it's correct. Read [Effective Go for more about Switches](https://golang.org/doc/effective_go.html#switch)
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
+ doFnName := reflectx.FunctionName(doFn)
+ doFnOutSize := reflectx.FunctionOutputSize(doFn)
+
+ parDoName := reflectx.FunctionName(parDo)
+ parDoOutSize := reflectx.FunctionOutputSize(parDo)
+
+ useParDo := reflectx.FunctionName(RecommendParDo(doFnOutSize))
+ return fmt.Sprintf("DoFn %v has %v outptus, but %v requires %v outputs, Use %v instead.", doFnName, doFnOutSize, parDoName, parDoOutSize, useParDo)
+
+}
+
+// recommendParDo takes a in a DoFns emit dimension and recommends the correct
+// ParDo to use.
+func RecommendParDo(emitDim int) interface{} {
Review comment:
Same comment here, WRT exported functions being part of the external API. Please rename this to recommendParDo.
##########
File path: sdks/go/test/regression/pardo_test.go
##########
@@ -16,6 +16,8 @@
package regression
Review comment:
Consider putting this into the beam_test package rather than in the go regression test package. This package is intended for bugs when interacting with runners, which is harder to test independantly in the Go unit tests.
In particular, I recommend adding a new test file:
.../sdks/go/pkg/beam/pardo_test.go
The main "gotcha" is you should copy the License header to the top of the file, as that's a requirement for the project.
##########
File path: sdks/go/test/regression/pardo_test.go
##########
@@ -56,3 +58,39 @@ func TestEmitParDoAfterGBK(t *testing.T) {
t.Error(err)
}
}
+
+// Keep the ParDo misalignment messages concise.
+// FIXME: Review
+// [beam-10169] identified that the error message returned to the user when the
+// DoFn output is misaligned with the ParDo output was unclear. in order to
+// make the user debug experience shorter, a more concise error message is
+// required.
+//
+// This suite of brittle tests are to ensure that the returned errors are adhere
+// to the ParDo API provided by the go sdk.
+
+func TestRecommendParDoWithOneOutput(t *testing.T) {
Review comment:
Following from the comment I made before, if this test remains stand alone, TestRecommendParDo_OneOutput is the idiomatic name. The general form is Test<Func/struct under test>_<case>
##########
File path: sdks/go/pkg/beam/core/util/reflectx/functions.go
##########
@@ -45,3 +45,13 @@ func LoadFunction(ptr uintptr, t reflect.Type) interface{} {
*(*unsafe.Pointer)(unsafe.Pointer(v.Addr().Pointer())) = unsafe.Pointer(p)
return v.Interface()
}
+
+//FunctionOutputSize returns the dimensions of the output of a function.
+// Panics if the type is not a function.
+func FunctionOutputSize(fn interface{}) int {
Review comment:
Per other comments, I don't think this function is necessary, as fun and cool reflection is, using it should be avoided unless impossible because it complicates maintenance.
1. DoFns can be structs instead, with a ProcessElement method.
2. DoFn outputs in a function signature aren't directly related to the function outputs, so this is incorrect.
DoFns can have "emitter functions" as parameters, which are used to output 0 or more values to the Nth PCollection of the ParDo. The main return (if one exists) is always the first PCollection, and they proceed from there. Similarly
eg. a function with the signature
`func(K,V string, e1, e2, e3 func(string)) (string, string)` would require a ParDo4 since it takes a PCollection<KV<string,string>> as an input, and has 4 outputs.
The first output is a KV<string,string>, and then there are 3 PCollection<string> as well.
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
Review comment:
In Go, Identifiers that start with a capital letter are Exported and accessible by code importing the package. As such Exported functions form the packages external API. Not that doesn't prevent the function from being unit tested in the associated _test.go file.
We don't want this function as part of the Beam API surface, as it's an internal implementation detail, that isn't necessary for users to invoke directly.
Further, the "er" bit idiomatically implies that it's an interface. Eg. Closer, Writer, Reader which are interfaces for values with a Close, Write, and Read methods respectively.
So with all that together, consider renaming it formatParDoError.
##########
File path: sdks/go/pkg/beam/core/util/reflectx/functions.go
##########
@@ -45,3 +45,13 @@ func LoadFunction(ptr uintptr, t reflect.Type) interface{} {
*(*unsafe.Pointer)(unsafe.Pointer(v.Addr().Pointer())) = unsafe.Pointer(p)
return v.Interface()
}
+
+//FunctionOutputSize returns the dimensions of the output of a function.
Review comment:
Style wise, you should have a space after the // and before the function name.
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
+ doFnName := reflectx.FunctionName(doFn)
+ doFnOutSize := reflectx.FunctionOutputSize(doFn)
+
+ parDoName := reflectx.FunctionName(parDo)
+ parDoOutSize := reflectx.FunctionOutputSize(parDo)
+
+ useParDo := reflectx.FunctionName(RecommendParDo(doFnOutSize))
+ return fmt.Sprintf("DoFn %v has %v outptus, but %v requires %v outputs, Use %v instead.", doFnName, doFnOutSize, parDoName, parDoOutSize, useParDo)
+
+}
+
+// recommendParDo takes a in a DoFns emit dimension and recommends the correct
+// ParDo to use.
+func RecommendParDo(emitDim int) interface{} {
+ switch {
+ case emitDim == 0:
+ return ParDo0
+ case emitDim == 1:
+ return ParDo
+ case emitDim == 2:
+ return ParDo2
+ case emitDim == 3:
+ return ParDo3
+ case emitDim == 4:
+ return ParDo4
+ case emitDim == 5:
+ return ParDo5
+ case emitDim == 6:
+ return ParDo6
+ case emitDim == 7:
+ return ParDo7
+ }
+ return ParDoN
+}
Review comment:
Switch statements in Go can accept a value, so this can be rewritten as
```suggestion
switch emitDim {
case 0:
return ParDo0
case 1:
return ParDo
case 2:
return ParDo2
case 3:
return ParDo3
case 4:
return ParDo4
case 5:
return ParDo5
case 6:
return ParDo6
case 7:
return ParDo7
}
return ParDoN
}
```
##########
File path: sdks/go/pkg/beam/core/util/reflectx/functions_test.go
##########
@@ -41,3 +45,19 @@ func TestLoadFunction(t *testing.T) {
t.Errorf("got %d, wanted %d", out[0].Int(), testFunction())
}
}
+
+func TestFunctionOutputSize(t *testing.T) {
+ expected := 1
+ received := FunctionOutputSize(testFunction)
+ if received != expected {
+ t.Errorf("got %d, wanted %d", received, expected)
+ }
+}
+
+func TestFunction2OutputSize(t *testing.T) {
+ expected := 2
+ received := FunctionOutputSize(testFunction2)
Review comment:
Idiomatically, Go uses "got and want" as the variables, as well as in the test error output.
##########
File path: sdks/go/pkg/beam/core/util/reflectx/functions_test.go
##########
@@ -41,3 +45,19 @@ func TestLoadFunction(t *testing.T) {
t.Errorf("got %d, wanted %d", out[0].Int(), testFunction())
}
}
+
+func TestFunctionOutputSize(t *testing.T) {
+ expected := 1
+ received := FunctionOutputSize(testFunction)
+ if received != expected {
+ t.Errorf("got %d, wanted %d", received, expected)
+ }
+}
+
+func TestFunction2OutputSize(t *testing.T) {
+ expected := 2
+ received := FunctionOutputSize(testFunction2)
+ if received != expected {
+ t.Errorf("got %d, wanted %d", received, expected)
Review comment:
It should be possible for a user to not look at the test code to understand what happened for this test.
So a better test output would be:
t.Errorf("FunctionOutputSize(%v) = %d, want %d", <testfunctionname/signature>,received, expected)
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
+ doFnName := reflectx.FunctionName(doFn)
+ doFnOutSize := reflectx.FunctionOutputSize(doFn)
+
+ parDoName := reflectx.FunctionName(parDo)
+ parDoOutSize := reflectx.FunctionOutputSize(parDo)
+
+ useParDo := reflectx.FunctionName(RecommendParDo(doFnOutSize))
+ return fmt.Sprintf("DoFn %v has %v outptus, but %v requires %v outputs, Use %v instead.", doFnName, doFnOutSize, parDoName, parDoOutSize, useParDo)
Review comment:
typo: outputs
The "Use" should be "use",
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
Review comment:
Note that regardless of the DoFn we need to check the expectations for every value against a given constant.
Consider wrapping the whole if block in the new function instead, and passing in the DoFn (so we can get the name), ret for the number of outputs, and the expected number of outputs from the constant. The function can be called mustHaveOutputs() . This way we can avoid specifying the expected number of outputs twice (in the if condition, and in the message) because it'll be in a function parameter.
(In Go, a Must prefix indicates the function will panic if the operation fails, rather than return an error. Usually they're discouraged, but in this case we have no choice but to panic. The ParDo functions could be described as Must functions, but that would get tiring WRT pipeline construction very quickly, so the shorter version without must, but with a Try prefix variant for the one Error handling version (TryParDo) was chosen instead.)
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
+ doFnName := reflectx.FunctionName(doFn)
Review comment:
DoFns in the Go SDK can be Structs instead of funcs, which means these functions will panic with less helpful error messages since they won't be functions.
However, we know about this already, so we have a suite of Go SDK internal functions to handle this. In this case You want graph.Fn which returns a graph.Fn which has a Name() method which can be used to get the DoFn's 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] codeBehindMe removed a comment on pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe removed a comment on pull request #11976:
URL: https://github.com/apache/beam/pull/11976#issuecomment-642290142
@damondouglas Hey mate, could you give this a quick once over.
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440531918
##########
File path: sdks/go/pkg/beam/pardo_test.go
##########
@@ -0,0 +1,54 @@
+// 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 beam
+
+import (
+ "testing"
+)
+
+func TestRecommendParDo(t *testing.T) {
+ var tests = []struct {
+ name string
+ outputDim int
+ want string
+ }{
+ {"zero outputs", 0, "ParDo0"},
+ {"one output", 1, "ParDo"},
Review comment:
Will do. Cheers.
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r439726602
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
+ doFnName := reflectx.FunctionName(doFn)
+ doFnOutSize := reflectx.FunctionOutputSize(doFn)
+
+ parDoName := reflectx.FunctionName(parDo)
+ parDoOutSize := reflectx.FunctionOutputSize(parDo)
+
+ useParDo := reflectx.FunctionName(RecommendParDo(doFnOutSize))
+ return fmt.Sprintf("DoFn %v has %v outptus, but %v requires %v outputs, Use %v instead.", doFnName, doFnOutSize, parDoName, parDoOutSize, useParDo)
+
+}
+
+// recommendParDo takes a in a DoFns emit dimension and recommends the correct
+// ParDo to use.
+func RecommendParDo(emitDim int) interface{} {
+ switch {
+ case emitDim == 0:
+ return ParDo0
+ case emitDim == 1:
+ return ParDo
+ case emitDim == 2:
+ return ParDo2
+ case emitDim == 3:
+ return ParDo3
+ case emitDim == 4:
+ return ParDo4
+ case emitDim == 5:
+ return ParDo5
+ case emitDim == 6:
+ return ParDo6
+ case emitDim == 7:
+ return ParDo7
+ }
+ return ParDoN
+}
Review comment:
db99bf446a
----------------------------------------------------------------
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 #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
lostluck commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r439765118
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
+ doFnName := reflectx.FunctionName(doFn)
Review comment:
I see where the confusion is coming in. What I meant was to call `graph.NewFn(doFn)` which will return the `graph.Fn` value which you can then call `Name()` on.
https://github.com/apache/beam/blob/master/sdks/go/pkg/beam/core/graph/fn.go#L78
At the point you're calling it, you already know it's going to be valid (since it was called by the TryParDo earlier). Go isn't magic, and won't change the type of an interface{} value unless assigned explicitly (which won't happen in this scope). interface{} values are passed by value, so even if you change the type them they'll remain the same in the outer scope: See an example here https://play.golang.org/p/A5QDrGyRwUu
----------------------------------------------------------------
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 #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
lostluck commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r439765234
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -428,31 +428,20 @@ func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
parDoName := reflectx.FunctionName(parDo)
parDoOutSize := reflectx.FunctionOutputSize(parDo)
- useParDo := reflectx.FunctionName(RecommendParDo(doFnOutSize))
+ useParDo := reflectx.FunctionName(recommendParDo(doFnOutSize))
Review comment:
Note that now that you've changed the return type of `recommendParDo`, you don't need to call `reflectx.FunctionName` anymore.
##########
File path: sdks/go/pkg/beam/pardo_test.go
##########
@@ -0,0 +1,40 @@
+// 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 beam
+
+import "testing"
+
+func TestRecommendParDo(t *testing.T) {
+ var tests = []struct {
+ name string
+ outputDim int
+ want string
+ }{
+ {"zero outputs", 0, "ParDo0"},
+ {"one output", 1, "ParDo"},
+ {"more than 7 outputs", 10, "ParDoN"},
+ }
+
+ for _, tt := range tests {
+ testName := tt.name
+ t.Run(testName, func(t *testing.T) {
+ got := recommendParDo(tt.outputDim)
+ if got != tt.want {
+ t.Errorf("RecommendParDo(%v) = %v, want %v", tt.outputDim, got, tt.want)
Review comment:
The name needs to be updated here: recommendParDo
----------------------------------------------------------------
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] codeBehindMe commented on pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on pull request #11976:
URL: https://github.com/apache/beam/pull/11976#issuecomment-642339945
R: @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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r439729887
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +414,45 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(ParDoErrorFormatter(dofn, ParDo7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//ParDoErrorFormatter is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+func ParDoErrorFormatter(doFn interface{}, parDo interface{}) string {
+ doFnName := reflectx.FunctionName(doFn)
Review comment:
I used the `FunctionName` from the `reflectx` package because what's passed through to `ParDo` is a `interface{}` type.
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440725797
##########
File path: sdks/go/pkg/beam/pardo_test.go
##########
@@ -0,0 +1,54 @@
+// 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 beam
+
+import (
+ "testing"
+)
+
+func TestRecommendParDo(t *testing.T) {
+ var tests = []struct {
+ name string
+ outputDim int
+ want string
+ }{
+ {"zero outputs", 0, "ParDo0"},
+ {"one output", 1, "ParDo"},
+ {"more than 7 outputs", 10, "ParDoN"},
+ }
+
+ for _, tt := range tests {
+ testName := tt.name
+ t.Run(testName, func(t *testing.T) {
+ got := recommendParDo(tt.outputDim)
+ if got != tt.want {
+ t.Errorf("RecommendParDo(%v) = %v, want %v", tt.outputDim, got, tt.want)
+ }
+ })
+ }
+}
+
+func testFunction() int64 {
Review comment:
0affb73655
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +412,36 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(formatParDoError(dofn, len(ret), 7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//formatParDoError is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+//
+// We construct a new graph.Fn using the doFn which is passed. We explicitly
+// ignore the error since we already know that its already a DoFn type as
+// TryParDo would have panicked otherwise.
+func formatParDoError(doFn interface{}, emitSize int, parDoSize int) string {
+ doFun, _ := graph.NewFn(doFn)
+ doFnName := doFun.Name()
+
+ thisParDo := recommendParDo(parDoSize) // Conveniently keeps the API slim.
Review comment:
b31ab3a885
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r439723527
##########
File path: sdks/go/pkg/beam/core/util/reflectx/functions_test.go
##########
@@ -41,3 +45,19 @@ func TestLoadFunction(t *testing.T) {
t.Errorf("got %d, wanted %d", out[0].Int(), testFunction())
}
}
+
+func TestFunctionOutputSize(t *testing.T) {
+ expected := 1
+ received := FunctionOutputSize(testFunction)
+ if received != expected {
+ t.Errorf("got %d, wanted %d", received, expected)
+ }
+}
+
+func TestFunction2OutputSize(t *testing.T) {
Review comment:
I had a look at the go by example, was very helpful. Changes in a4d5083dbf .
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440723127
##########
File path: sdks/go/pkg/beam/pardo.go
##########
@@ -414,7 +412,36 @@ func ParDo6(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollec
func ParDo7(s Scope, dofn interface{}, col PCollection, opts ...Option) (PCollection, PCollection, PCollection, PCollection, PCollection, PCollection, PCollection) {
ret := MustN(TryParDo(s, dofn, col, opts...))
if len(ret) != 7 {
- panic(fmt.Sprintf("expected 7 output. Found: %v", ret))
+ panic(formatParDoError(dofn, len(ret), 7))
}
return ret[0], ret[1], ret[2], ret[3], ret[4], ret[5], ret[6]
}
+
+//formatParDoError is a helper function to provide a more concise error
+// message to the users when a DoFn and its ParDo pairing is incorrect.
+//
+// We construct a new graph.Fn using the doFn which is passed. We explicitly
+// ignore the error since we already know that its already a DoFn type as
+// TryParDo would have panicked otherwise.
+func formatParDoError(doFn interface{}, emitSize int, parDoSize int) string {
+ doFun, _ := graph.NewFn(doFn)
+ doFnName := doFun.Name()
+
+ thisParDo := recommendParDo(parDoSize) // Conveniently keeps the API slim.
Review comment:
b31ab3a885
----------------------------------------------------------------
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] codeBehindMe commented on a change in pull request #11976: [BEAM-10169] - ParDo functions with correct output N in their error messages.
Posted by GitBox <gi...@apache.org>.
codeBehindMe commented on a change in pull request #11976:
URL: https://github.com/apache/beam/pull/11976#discussion_r440107710
##########
File path: sdks/go/pkg/beam/pardo_test.go
##########
@@ -0,0 +1,40 @@
+// 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 beam
+
+import "testing"
+
+func TestRecommendParDo(t *testing.T) {
+ var tests = []struct {
+ name string
+ outputDim int
+ want string
+ }{
+ {"zero outputs", 0, "ParDo0"},
+ {"one output", 1, "ParDo"},
+ {"more than 7 outputs", 10, "ParDoN"},
+ }
+
+ for _, tt := range tests {
+ testName := tt.name
+ t.Run(testName, func(t *testing.T) {
+ got := recommendParDo(tt.outputDim)
+ if got != tt.want {
+ t.Errorf("RecommendParDo(%v) = %v, want %v", tt.outputDim, got, tt.want)
Review comment:
45f4a84e57
----------------------------------------------------------------
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