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 2022/06/02 19:58:35 UTC
[GitHub] [beam] y1chi opened a new pull request, #17818: [BEAM-14553] Add destination coder to FileResultCoder components
y1chi opened a new pull request, #17818:
URL: https://github.com/apache/beam/pull/17818
This change ensures that FileResultCoders with different destination coder component can be registered individually. Otherwise only the first FileResultCode will be registered successfully and later on be wrongly reused for other FileResultCoder that should use a different destination coder.
------------------------
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).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on PR #17818:
URL: https://github.com/apache/beam/pull/17818#issuecomment-1150260079
Run Java 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on PR #17818:
URL: https://github.com/apache/beam/pull/17818#issuecomment-1154171267
Run Java 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r888423143
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
Should it be FileResult<BoundedWindowT, DestinationT> and the windowCoder being Coder<BoundedWindowT extends BoundedWindow>? Looking from the code it think the window coder can change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r889158954
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
So IIUC, the right thing to do is to have FileResult<WindowT, DestinationT> so that coder inference can work for both window type and destination type. But since all FileResult PCollections use explicit setCoders that's not a requirement. I still think window coder needs to be in the components so that FileResultCoder with different window coders won't collide with each other.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kennknowles commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
kennknowles commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r888403792
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
It seems to me that both versions are wrong and it should be `ImmutableList.of(destinationCoder)`.
The javadoc:
```
/**
* If this is a {@link Coder} for a parameterized type, returns the list of {@link Coder}s being
* used for each of the parameters in the same order they appear within the parameterized type's
* type signature. If this cannot be done, or this {@link Coder} does not encode/decode a
* parameterized type, returns the empty list.
*/
```
This means that since it is a `Coder<FileResult<DestinationT>>` that the return value from `getCoderArguments` should line up with `DestinationT`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on PR #17818:
URL: https://github.com/apache/beam/pull/17818#issuecomment-1151498377
@kennknowles java precommit failure seems unrelated, could you take another look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on PR #17818:
URL: https://github.com/apache/beam/pull/17818#issuecomment-1145286152
R: @kennknowles
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r888423143
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
Should it be `FileResult<BoundedWindowT extends BoundedWindow, DestinationT>` and the windowCoder being `Coder<BoundedWindowT>` Looking from the code it think the window coder can change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kennknowles commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
kennknowles commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r896900888
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
Yes, I think 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on PR #17818:
URL: https://github.com/apache/beam/pull/17818#issuecomment-1146175229
Run Java 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on PR #17818:
URL: https://github.com/apache/beam/pull/17818#issuecomment-1145353805
> Any idea why this worked previously for Dataflow v1 ?
I'm not quite sure but I guess the non-portable job submission have a different mechanism to register coders?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r888423143
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
Should it be `FileResult<BoundedWindowT, DestinationT>` and the windowCoder being `Coder<BoundedWindowT extends BoundedWindow>` Looking from the code it think the window coder can change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r888463072
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
When registering a StructuredCoder, the component coders are hashed and compared in order to avoid duplicate. That means if we have one FileResultCoder(IntervalWindowCoder, VoidCoder) and another FileResultCoder(GlobalWIndowCoder, VoidCoder) they'll still cause collision if we use Arrays.asList(destinationCoder) as components.
I didn't see the window coder is being used to encode/decode the destinationT, the first registered FileResultCoder has serialized payload and then deserialized correctly and was able to decode the FileResult type correctly. But the other FileResultCoder will be deserialized from the same payload will throw exception because the destination coder does not match.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r888468027
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
getTypeToCoderBindings seems to be used when we need to infer coder from parameterized type? but it looks like we always call setCoder(FileResultCoder) at PCollections with FileResult 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] chamikaramj commented on pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
chamikaramj commented on PR #17818:
URL: https://github.com/apache/beam/pull/17818#issuecomment-1145297289
Any idea why this worked previously for Dataflow v1 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi merged pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi merged PR #17818:
URL: https://github.com/apache/beam/pull/17818
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kennknowles commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
kennknowles commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r888443505
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
You can see how it is used here: https://github.com/apache/beam/blob/243128a8fc52798e1b58b0cf1a271d95ee7aa241/sdks/java/core/src/main/java/org/apache/beam/sdk/coders/CoderRegistry.java#L723
So if you have a `Coder<Foo<A, B, C>>` then the list of type arguments is `A, B, C` and `getCoderArguments` needs to return a `Coder<A>, Coder<B>, Coder<C>` that matches. So definitely before this pull request the coder registry will try to encode `DestinationT` with the window coder.
Right now `FileResult` just holds the window without tracking the type variable for it. I think it would be a breaking change to make it `FileResult<WindowT, DestinationT>`. I think just changing the window coder to the destination coder is the right thing to 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kennknowles commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
kennknowles commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r888472398
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
Yes, to be clear, this is only used for automatically inferring coder from input to output. So like if you have `FileResult<DestinationT>` and you map it to `DestinationT` then just for that transform the coder will be automatically determined. Or at least that is the intent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r891702392
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
On a side note, I don't feel like the default behavior of getComponents() of StructuredCoder should just return whatever getCoderArguments() provides, this feels error-prone. Should we make getComponents a abstract method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r888463072
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
When registering a StructuredCoder, the component coders are hashed and compared in order to avoid duplicate. That means if we have one FileResultCoder(IntervalWindowCoder, VoidCoder) and another FileResultCoder(GlobalWIndowCoder, VoidCoder) they'll still cause collision.
I didn't see the window coder is being used to encode/decode the destinationT, the first registered FileResultCoder has serialized payload and then deserialized correctly and was able to decode the FileResult type correctly. But the other FileResultCoder will be deserialized from the same payload will throw exception because the destination coder does not match.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on PR #17818:
URL: https://github.com/apache/beam/pull/17818#issuecomment-1146332642
Run Java 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on PR #17818:
URL: https://github.com/apache/beam/pull/17818#issuecomment-1149075323
Run Java 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] y1chi commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
y1chi commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r891676464
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
Ah, that makes sense, update the 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kennknowles commented on pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
kennknowles commented on PR #17818:
URL: https://github.com/apache/beam/pull/17818#issuecomment-1155273634
run java 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] kennknowles commented on a diff in pull request #17818: [BEAM-14553] Add destination coder to FileResultCoder components
Posted by GitBox <gi...@apache.org>.
kennknowles commented on code in PR #17818:
URL: https://github.com/apache/beam/pull/17818#discussion_r891658075
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java:
##########
@@ -1196,7 +1196,7 @@ public static <DestinationT> FileResultCoder<DestinationT> of(
@Override
public List<? extends Coder<?>> getCoderArguments() {
- return Arrays.asList(windowCoder);
+ return Arrays.asList(windowCoder, destinationCoder);
Review Comment:
Yes, I think what you are describing makes sense. I do not know why we decided to not have a generic parameter for `<WindowT>`. Now it would be backwards-incompatible.
Note that `getComponents` is part of `StructuredCoder`. They overlap but are different. The things in `getComponents` should be any coder that can be passed to the constructor to change the behavior of the coder. Often there is no type variable.
So based on https://github.com/apache/beam/blob/e95ef975a3cb2f2562493ff2e0943d3e000376dc/sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java#L1187 the return value of `getComponents()` should be `windowCoder, destinationCoder` but because of https://github.com/apache/beam/blob/e95ef975a3cb2f2562493ff2e0943d3e000376dc/sdks/java/core/src/main/java/org/apache/beam/sdk/io/FileBasedSink.java#L1178 the return value of `getCoderArguments` should be just `destinationCoder`.
This class is actually a perfect example of why those two methods are different!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org