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/07/15 00:56:15 UTC
[GitHub] [beam] steveniemitz opened a new pull request, #22289: Fix overly aggressive null check in RowWriterFactory
steveniemitz opened a new pull request, #22289:
URL: https://github.com/apache/beam/pull/22289
See #22288 for details
R: @kennknowles
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [x] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [x] 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] steveniemitz commented on a diff in pull request #22289: Fix overly aggressive null check in RowWriterFactory
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on code in PR #22289:
URL: https://github.com/apache/beam/pull/22289#discussion_r928869446
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/RowWriterFactory.java:
##########
@@ -135,10 +136,12 @@ BigQueryRowWriter<ElementT> createRowWriter(String tempFilePrefix, DestinationT
throw new IllegalStateException(
"createRowWriter called when schemaFactory is null; forgot to call prepare()?");
}
+ @SuppressWarnings({
Review Comment:
ah ok I see what you mean, I didn't realize you could annotate generic parameters like that as well. Updated.
--
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] steveniemitz commented on a diff in pull request #22289: Fix overly aggressive null check in RowWriterFactory
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on code in PR #22289:
URL: https://github.com/apache/beam/pull/22289#discussion_r928061849
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/RowWriterFactory.java:
##########
@@ -135,10 +136,12 @@ BigQueryRowWriter<ElementT> createRowWriter(String tempFilePrefix, DestinationT
throw new IllegalStateException(
"createRowWriter called when schemaFactory is null; forgot to call prepare()?");
}
+ @SuppressWarnings({
Review Comment:
given that `schemaFactory` here is a `SerializableFunction`, I don't think that's really feasible. Doing so (changing SerializableFunction.apply to take a nullable input) would cause a large amount of checker failures elsewhere, and changing the interface would be a breaking API change.
Did you have something else in mind?
--
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 #22289: Fix overly aggressive null check in RowWriterFactory
Posted by GitBox <gi...@apache.org>.
kennknowles commented on code in PR #22289:
URL: https://github.com/apache/beam/pull/22289#discussion_r927920479
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/RowWriterFactory.java:
##########
@@ -135,10 +136,12 @@ BigQueryRowWriter<ElementT> createRowWriter(String tempFilePrefix, DestinationT
throw new IllegalStateException(
"createRowWriter called when schemaFactory is null; forgot to call prepare()?");
}
+ @SuppressWarnings({
Review Comment:
Do not suppress the warning, but rather follow through the consequences of allowing null 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.
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 merged pull request #22289: Fix overly aggressive null check in RowWriterFactory
Posted by GitBox <gi...@apache.org>.
kennknowles merged PR #22289:
URL: https://github.com/apache/beam/pull/22289
--
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 #22289: Fix overly aggressive null check in RowWriterFactory
Posted by GitBox <gi...@apache.org>.
kennknowles commented on code in PR #22289:
URL: https://github.com/apache/beam/pull/22289#discussion_r929442458
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/RowWriterFactory.java:
##########
@@ -135,10 +136,12 @@ BigQueryRowWriter<ElementT> createRowWriter(String tempFilePrefix, DestinationT
throw new IllegalStateException(
"createRowWriter called when schemaFactory is null; forgot to call prepare()?");
}
+ @SuppressWarnings({
Review Comment:
Yea it is new in more recent versions of Java, and you have to use the checkerframework annotation because the one in the stdlib does not allow TYPE_USE occurrences. 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] steveniemitz commented on a diff in pull request #22289: Fix overly aggressive null check in RowWriterFactory
Posted by GitBox <gi...@apache.org>.
steveniemitz commented on code in PR #22289:
URL: https://github.com/apache/beam/pull/22289#discussion_r928061849
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/RowWriterFactory.java:
##########
@@ -135,10 +136,12 @@ BigQueryRowWriter<ElementT> createRowWriter(String tempFilePrefix, DestinationT
throw new IllegalStateException(
"createRowWriter called when schemaFactory is null; forgot to call prepare()?");
}
+ @SuppressWarnings({
Review Comment:
given that `schemaFactory` here is a `SerializableFunction`, I don't think that's really feasible. Doing so would cause a large amount of checker failures elsewhere, and changing the interface would be a breaking API change.
Did you have something else in mind?
--
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 #22289: Fix overly aggressive null check in RowWriterFactory
Posted by GitBox <gi...@apache.org>.
kennknowles commented on code in PR #22289:
URL: https://github.com/apache/beam/pull/22289#discussion_r928358362
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/RowWriterFactory.java:
##########
@@ -135,10 +136,12 @@ BigQueryRowWriter<ElementT> createRowWriter(String tempFilePrefix, DestinationT
throw new IllegalStateException(
"createRowWriter called when schemaFactory is null; forgot to call prepare()?");
}
+ @SuppressWarnings({
Review Comment:
`SerializableFunction` is parametric in input type and output type, it doesn't need to change. What I'm not clear on is whether it is safe to make `schemaFactory` have type `SerializableFunction<TableSchema, @Nullable Schema>`. I don't recall if that causes problems because there are places that do not properly check its return value.
--
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