-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Beam-6858] Validate that side-input parameters match the type of the PCollectionView #9372
Conversation
@@ -107,7 +108,8 @@ public void cleanup() throws Exception { | |||
List<PCollectionView<?>> sideInputs, | |||
TupleTag<OutputT> mainOutputTag, | |||
List<TupleTag<?>> additionalOutputTags, | |||
DoFnSchemaInformation doFnSchemaInformation) | |||
DoFnSchemaInformation doFnSchemaInformation, | |||
Map<String, String> sideInputMapping) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reuvenlax This should be String, String? or PCollectionView
Yes, we should change to Map<String, PCollectionView<?>>
…On Mon, Aug 19, 2019 at 10:50 PM Salman Raza ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
runners/direct-java/src/main/java/org/apache/beam/runners/direct/ParDoEvaluatorFactory.java
<#9372 (comment)>:
> @@ -107,7 +108,8 @@ public void cleanup() throws Exception {
List<PCollectionView<?>> sideInputs,
TupleTag<OutputT> mainOutputTag,
List<TupleTag<?>> additionalOutputTags,
- DoFnSchemaInformation doFnSchemaInformation)
+ DoFnSchemaInformation doFnSchemaInformation,
+ Map<String, String> sideInputMapping)
@reuvenlax <https://github.com/reuvenlax> This should be String, String?
or PCollectionView
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#9372?email_source=notifications&email_token=AFAYJVOIDLSSQKSCWIFEWL3QFOA37A5CNFSM4IMWQU52YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCA45TQ#pullrequestreview-276942542>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFAYJVIGE5T2FH655BWGMJLQFOA37ANCNFSM4IMWQU5Q>
.
|
9c0002b
to
83dc627
Compare
run Flink ValidatesRunner |
Run Dataflow ValidatesRunner |
Run Portable_Python PreCommit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Just a couple of suggestions
sideInput.sideInputId()); | ||
TypeDescriptor<?> viewType = view.getViewFn().getTypeDescriptor(); | ||
|
||
// Currently check that the types exactly match, even if the types are convertible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there plans to handle convertible types? Should there be a JIRA thats linked here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the same issue with Element parameters. I'll check to see if we have a JIRA for this (I think a user actually filed one some time back).
.apply(ParDo.of(fn).withSideInput(sideInputTag1, sideInput1)); | ||
pipeline.run(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind adding tests for the remaining view types - Map, Multimap, Iterable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tests added
all comments addressed |
…parameters match the type of the PCollectionView
Validate that the type of a processElement side-input parameter matches that of the PCollectionView.
Note: Suppliers are used because often the TypeDescriptor is not Serializable, and so cannot be captured in the PCollectionView.