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 2021/12/17 09:59:50 UTC
[GitHub] [beam] RustedBones opened a new pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
RustedBones opened a new pull request #16271:
URL: https://github.com/apache/beam/pull/16271
Beam can run with different versions of avro at runtime. However, some fixes from avro 1.9 are not visible due to beam internals
* [AVRO-1891](https://issues.apache.org/jira/browse/AVRO-1891) is fixed by adding the conversion at compile time in the generated class' model. Beam does not currently read the `SpecificData` form the class.
* [BEAM-8329](https://issues.apache.org/jira/browse/BEAM-8329) ReflectData is not supposed to use the logical conversions by default. Users should either use a `CustomEncoding` with the `AvroEncode` annotation or manually add the conversion to their own `ReflectData` which is not possible everywhere in beam.
* [BEAM-9144](https://issues.apache.org/jira/browse/BEAM-9144) is a 'snowflake'. All other joda logical time conversions are still failing. (`date`, `time-millis`, `time-micros`, `timestampt-micros`). This should not be in the framework but in the user code
This PR aims to
* Keep both avro 1.8 and 1.9 support
* Use proper logical type conversions when using avro 1.9 specific data (`useReflectApi` must be disabled)
* Give users the possibility to provide their own avro data in the `DatumWriterFactory` and `DatumReaderFactory` used by beam internals
Needs discussion:
* `AvroCoder` should have constructor with `DatumWriterFactory` and `DatumReaderFactory` but this causes a serialization issue with the [`AvroCoderCloudObjectTranslator`](https://github.com/apache/beam/blob/master/runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/util/AvroCoderCloudObjectTranslator.java) whenre the object needs to be converted into a json form.
------------------------
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).
`ValidatesRunner` compliance status (on master branch)
--------------------------------------------------------
<table>
<thead>
<tr>
<th>Lang</th>
<th>ULR</th>
<th>Dataflow</th>
<th>Flink</th>
<th>Samza</th>
<th>Spark</th>
<th>Twister2</th>
</tr>
</thead>
<tbody>
<tr>
<td>Go</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon">
</a>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Samza/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Samza/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
</tr>
<tr>
<td>Java</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_ULR/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_ULR/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/badge/icon?subject=V1">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Streaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Streaming/lastCompletedBuild/badge/icon?subject=V1+Streaming">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/badge/icon?subject=V1+Java+11">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2_Streaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_VR_Dataflow_V2_Streaming/lastCompletedBuild/badge/icon?subject=V2+Streaming">
</a><br>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/badge/icon?subject=Java+8">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/badge/icon?subject=Java+11">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/badge/icon?subject=Portable">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/badge/icon?subject=Portable+Streaming">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/badge/icon">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Samza/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Samza/lastCompletedBuild/badge/icon?subject=Portable">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/badge/icon">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/badge/icon?subject=Portable">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/badge/icon?subject=Structured+Streaming">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/badge/icon">
</a>
</td>
</tr>
<tr>
<td>Python</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/badge/icon?subject=V1">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/badge/icon?subject=ValCont">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/badge/icon?subject=Portable">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Samza/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Samza/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
</tr>
<tr>
<td>XLang</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Samza/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Samza/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
</tr>
</tbody>
</table>
Examples testing status on various runners
--------------------------------------------------------
<table>
<thead>
<tr>
<th>Lang</th>
<th>ULR</th>
<th>Dataflow</th>
<th>Flink</th>
<th>Samza</th>
<th>Spark</th>
<th>Twister2</th>
</tr>
</thead>
<tbody>
<tr>
<td>Go</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
<tr>
<td>Java</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Cron/lastCompletedBuild/badge/icon?subject=V1">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Java11_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Examples_Dataflow_Java11_Cron/lastCompletedBuild/badge/icon?subject=V1+Java11">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java_Examples_Dataflow_V2/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java_Examples_Dataflow_V2/lastCompletedBuild/badge/icon?subject=V2">
</a><br>
</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
<tr>
<td>Python</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
<tr>
<td>XLang</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
</tbody>
</table>
Post-Commit SDK/Transform Integration Tests Status (on master branch)
------------------------------------------------------------------------------------------------
<table>
<thead>
<tr>
<th>Go</th>
<th>Java</th>
<th>Python</th>
</tr>
</thead>
<tbody>
<tr>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/badge/icon?subject=3.6">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/badge/icon?subject=3.7">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/badge/icon?subject=3.8">
</a>
</td>
</tr>
</tbody>
</table>
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
<table>
<thead>
<tr>
<th>---</th>
<th>Java</th>
<th>Python</th>
<th>Go</th>
<th>Website</th>
<th>Whitespace</th>
<th>Typescript</th>
</tr>
</thead>
<tbody>
<tr>
<td>Non-portable</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/badge/icon">
</a><br>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/badge/icon?subject=Tests">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/badge/icon?subject=Lint">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/badge/icon?subject=Docker">
</a><br>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/badge/icon?subject=Docs">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
</tr>
<tr>
<td>Portable</td>
<td>---</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>
<a href="https://ci-beam.apache.org/job/beam_PreCommit_GoPortable_Cron/lastCompletedBuild/">
<img alt="Build Status" src="https://ci-beam.apache.org/job/beam_PreCommit_GoPortable_Cron/lastCompletedBuild/badge/icon">
</a>
</td>
<td>---</td>
<td>---</td>
<td>---</td>
</tr>
</tbody>
</table>
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.
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] aromanenko-dev removed a comment on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
aromanenko-dev removed a comment on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1060633271
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] RustedBones commented on a change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r784913604
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroCoder.java
##########
@@ -272,26 +263,8 @@ public Schema get() {
}
}
- /**
- * A {@link Serializable} object that lazily supplies a {@link ReflectData} built from the
- * appropriate {@link ClassLoader} for the type encoded by this {@link AvroCoder}.
- */
- private static class SerializableReflectDataSupplier
- implements Serializable, Supplier<ReflectData> {
-
- private final Class<?> clazz;
-
- private SerializableReflectDataSupplier(Class<?> clazz) {
- this.clazz = clazz;
- }
-
- @Override
- public ReflectData get() {
- ReflectData reflectData = new ReflectData(clazz.getClassLoader());
- reflectData.addLogicalTypeConversion(new JodaTimestampConversion());
- return reflectData;
- }
- }
+ private final AvroSource.DatumReaderFactory<T> readerFactory;
+ private final AvroSink.DatumWriterFactory<T> writerFactory;
Review comment:
Solved using specialized implementations
--
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] aromanenko-dev commented on a change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r822913878
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java
##########
@@ -542,18 +550,21 @@
* <p>If the output type is {@link GenericRecord} use {@link #writeCustomTypeToGenericRecords()}
* instead.
*/
- public static <UserT, OutputT> TypedWrite<UserT, Void, OutputT> writeCustomType() {
- return AvroIO.<UserT, OutputT>defaultWriteBuilder().setGenericRecords(false).build();
+ public static <UserT, OutputT> TypedWrite<UserT, Void, OutputT> writeCustomType(
+ Class<OutputT> recordClass) {
Review comment:
I guess it was a question if we can avoid breaking changes here, isn't it?
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroReflectCoder.java
##########
@@ -0,0 +1,70 @@
+/*
+ * 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 org.apache.beam.sdk.coders;
+
+import org.apache.avro.Schema;
+import org.apache.avro.io.DatumReader;
+import org.apache.avro.io.DatumWriter;
+import org.apache.avro.reflect.Nullable;
+import org.apache.avro.reflect.ReflectData;
+import org.apache.avro.reflect.ReflectDatumReader;
+import org.apache.avro.reflect.ReflectDatumWriter;
+import org.apache.avro.reflect.Union;
+
+/**
+ * AvroCoder specialisation for avro classes using Java reflection.
+ *
+ * <p>Only concrete classes with a no-argument constructor can be mapped to Avro records. All
+ * inherited fields that are not static or transient are included. Fields are not permitted to be
+ * null unless annotated by {@link Nullable} or a {@link Union} schema containing {@code "null"}.
+ */
+public class AvroReflectCoder<T> extends AvroCoder<T> {
+
+ @SuppressWarnings("nullness") // new ReflectData(ClassLoader) is not annotated to accept null
+ public AvroReflectCoder(Class<T> type) {
Review comment:
Why do we need public constructors?
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroGenericCoder.java
##########
@@ -18,15 +18,31 @@
package org.apache.beam.sdk.coders;
import org.apache.avro.Schema;
+import org.apache.avro.generic.GenericDatumReader;
+import org.apache.avro.generic.GenericDatumWriter;
import org.apache.avro.generic.GenericRecord;
+import org.apache.avro.io.DatumReader;
+import org.apache.avro.io.DatumWriter;
/** AvroCoder specialisation for GenericRecord. */
public class AvroGenericCoder extends AvroCoder<GenericRecord> {
- AvroGenericCoder(Schema schema) {
+
+ public AvroGenericCoder(Schema schema) {
Review comment:
Why do we need this be `public`?
--
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] RustedBones commented on a change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r785021987
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/AvroRecordSchema.java
##########
@@ -32,12 +32,14 @@
* objects, creating a schema that matches that inferred by the AVRO libraries.
*/
@SuppressWarnings({
+ "nullness", // TODO(https://issues.apache.org/jira/browse/BEAM-10402)
Review comment:
`avroType.getClassLoader()` can return null apparently. It is done that way everywhere else but 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] RustedBones commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-996807940
R: @aromanenko-dev
--
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] aromanenko-dev commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1004862134
CC: @kennknowles @lukecwik @TheNeuralBit
I think that this PR brings some significant changes to `AvroCoder` and it should be discussed before.
--
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] RustedBones commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1025544434
On my side, I'd say the PR is ready and addresses the main pain points.
There are maybe some things to consider on the beam maintainer side:
- there are some small breaking changes in the avro API.
- `timestamp-millis` logical type conversion is not supported by default anymore and will require custom factories & coder.
--
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] aromanenko-dev commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1060633271
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] RustedBones commented on a change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r785032910
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/AvroRecordSchema.java
##########
@@ -32,12 +32,14 @@
* objects, creating a schema that matches that inferred by the AVRO libraries.
*/
@SuppressWarnings({
+ "nullness", // TODO(https://issues.apache.org/jira/browse/BEAM-10402)
Review comment:
No, the default constructor actually calls with `null`.
There is however no `@Nullable` annotation in the library. Seems the checker considers it as required.
--
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] RustedBones commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1060724353
Sure @aromanenko-dev!
> What are the actual breaking changes? Could you list them?
API breaking changes:
- `AvroCoder`: removing `useReflectApi()` which was introduced in 2.36.0 in favour of specialized implementations
- `AvroIO`: `writeCustomType` now must be provided with the class of the output record
- `JodaTimestampConversion` is removed
Runtime breaking changes:
- Current avro coder uses a custom `ReflectData`, initialized with `JodaTimestampConversion`. This PR will remove this and only use default avro's `GenericData`, `SpecificData` or `ReflectData`.
Any users that are using avro 1.9 would need to manually add joda conversion to be able to support schemas compiled with avro 1.8 (time logical types have been migrated to `java.time`).
Any users at use reflect data with `org.joda.time.DateTime` will have to either use the [`AvroEncode`](https://avro.apache.org/docs/1.8.2/api/java/org/apache/avro/reflect/AvroEncode.html) annotation or manually add joda conversion as done [here](https://github.com/apache/beam/blob/26261ecea8bd92b79989c5ed5c3c4efa06823ecf/sdks/java/core/src/test/java/org/apache/beam/sdk/coders/AvroCoderTest.java#L183)
> Are all review comments already addressed and, if not, could you elaborate why?
This [comment](https://github.com/apache/beam/pull/16271/files#r771263422) and this [one](https://github.com/apache/beam/pull/16271/files#r771265701) are not answered. I'm not sure I understood properly. I think the generic codepath is already covered by `writeCustomTypeToGenericRecords` in the 1st case. In the 2nd case the API aligns with the `Read` ones.
--
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] aromanenko-dev commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1059409659
--
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] aromanenko-dev commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1001600619
@RustedBones Sorry, I still didn't have a time for review but did you check a failed test? Is it related to your changes?
--
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 change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r785028496
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/AvroRecordSchema.java
##########
@@ -32,12 +32,14 @@
* objects, creating a schema that matches that inferred by the AVRO libraries.
*/
@SuppressWarnings({
+ "nullness", // TODO(https://issues.apache.org/jira/browse/BEAM-10402)
Review comment:
Does `ReflectData` require its argument to be non-null?
--
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] RustedBones commented on a change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r784862869
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroSink.java
##########
@@ -38,20 +42,52 @@
})
public class AvroSink<UserT, DestinationT, OutputT>
extends FileBasedSink<UserT, DestinationT, OutputT> {
- private final boolean genericRecords;
+ private final Class<OutputT> type;
Review comment:
I know but this has been called 'type' everywhere in the Avro section (`AvroCoder` & `AvroWriter` and `AvroSource.Mode`). I'm just keeping the consistency
--
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 change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r781527428
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroCoder.java
##########
@@ -182,7 +173,25 @@ public static AvroGenericCoder of(Schema schema) {
* @param <T> the element type
*/
public static <T> AvroCoder<T> of(Class<T> type, Schema schema, boolean useReflectApi) {
- return new AvroCoder<>(type, schema, useReflectApi);
+ AvroSource.DatumReaderFactory<T> readerFactory =
+ new AvroSource.DefaultDatumReaderFactory<>(type, useReflectApi);
+ AvroSink.DatumWriterFactory<T> writerFactory =
+ new AvroSink.DefaultDatumWriterFactory<>(type, useReflectApi);
+ return new AvroCoder<>(type, schema, readerFactory, writerFactory);
+ }
+
+ /**
+ * Returns an {@code AvroCoder} instance for the given class and schema, using the provided datum
+ * factories.
+ *
+ * @param <T> the element type
+ */
+ public static <T> AvroCoder<T> of(
+ Class<T> type,
+ Schema schema,
+ AvroSource.DatumReaderFactory<T> readerFactory,
Review comment:
A little thing: I think `AvroSource` and `AvroSink` can depend on `AvroCoder` but it should not necessarily go the other way around. Since this is just a type alias functional interface, you could just have an interface in this file, or an interface independent of both of them.
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java
##########
@@ -542,18 +550,21 @@
* <p>If the output type is {@link GenericRecord} use {@link #writeCustomTypeToGenericRecords()}
* instead.
*/
- public static <UserT, OutputT> TypedWrite<UserT, Void, OutputT> writeCustomType() {
- return AvroIO.<UserT, OutputT>defaultWriteBuilder().setGenericRecords(false).build();
+ public static <UserT, OutputT> TypedWrite<UserT, Void, OutputT> writeCustomType(
+ Class<OutputT> recordClass) {
Review comment:
Can you make it into two codepaths? I am just coming to this PR to help out a little so I don't have all the history of your thoughts on this.
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroSink.java
##########
@@ -38,20 +42,52 @@
})
public class AvroSink<UserT, DestinationT, OutputT>
extends FileBasedSink<UserT, DestinationT, OutputT> {
- private final boolean genericRecords;
+ private final Class<OutputT> type;
Review comment:
nit: a class is slightly different from a type (types have generics and constraints, etc, etc) so prefer some identifier like `elementClass` or `clazz`
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java
##########
@@ -1338,7 +1383,7 @@ public void populateDisplayData(DisplayData.Builder builder) {
abstract Builder<UserT, DestinationT, OutputT> setShardTemplate(
@Nullable String shardTemplate);
- abstract Builder<UserT, DestinationT, OutputT> setGenericRecords(boolean genericRecords);
+ abstract Builder<UserT, DestinationT, OutputT> setRecordClass(Class<OutputT> recordClass);
Review comment:
Can you make this also a separate code path?
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroCoder.java
##########
@@ -301,56 +292,42 @@ public ReflectData get() {
private final EmptyOnDeserializationThreadLocal<DatumWriter<T>> writer;
private final EmptyOnDeserializationThreadLocal<DatumReader<T>> reader;
- // Lazily re-instantiated after deserialization
- private final Supplier<ReflectData> reflectData;
-
- protected AvroCoder(Class<T> type, Schema schema) {
- this(type, schema, false);
- }
-
- protected AvroCoder(Class<T> type, Schema schema, boolean useReflectApi) {
+ protected AvroCoder(
+ Class<T> type,
+ Schema schema,
+ AvroSource.DatumReaderFactory<T> readerFactory,
+ AvroSink.DatumWriterFactory<T> writerFactory) {
this.type = type;
- this.useReflectApi = useReflectApi;
+ this.useReflectApi = true; // TODO this is wrong
Review comment:
Just noting the TODO being unresolved
--
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] RustedBones commented on a change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r771257284
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroCoder.java
##########
@@ -272,26 +263,8 @@ public Schema get() {
}
}
- /**
- * A {@link Serializable} object that lazily supplies a {@link ReflectData} built from the
- * appropriate {@link ClassLoader} for the type encoded by this {@link AvroCoder}.
- */
- private static class SerializableReflectDataSupplier
- implements Serializable, Supplier<ReflectData> {
-
- private final Class<?> clazz;
-
- private SerializableReflectDataSupplier(Class<?> clazz) {
- this.clazz = clazz;
- }
-
- @Override
- public ReflectData get() {
- ReflectData reflectData = new ReflectData(clazz.getClassLoader());
- reflectData.addLogicalTypeConversion(new JodaTimestampConversion());
- return reflectData;
- }
- }
+ private final AvroSource.DatumReaderFactory<T> readerFactory;
+ private final AvroSink.DatumWriterFactory<T> writerFactory;
Review comment:
Ideally I would have liked to have those as constructor parameter.
As explained in the PR comment, this cases problem with the [`AvroCoderCloudObjectTranslator`](https://github.com/apache/beam/blob/master/runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/util/AvroCoderCloudObjectTranslator.java) which serializes the `AvroCoder` in a CloudObject (json). I don't know how to serialize the factories in that case
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java
##########
@@ -1338,7 +1383,7 @@ public void populateDisplayData(DisplayData.Builder builder) {
abstract Builder<UserT, DestinationT, OutputT> setShardTemplate(
@Nullable String shardTemplate);
- abstract Builder<UserT, DestinationT, OutputT> setGenericRecords(boolean genericRecords);
+ abstract Builder<UserT, DestinationT, OutputT> setRecordClass(Class<OutputT> recordClass);
Review comment:
This is a breaking change. It aligns better with the `Source` implementation.
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java
##########
@@ -542,18 +550,21 @@
* <p>If the output type is {@link GenericRecord} use {@link #writeCustomTypeToGenericRecords()}
* instead.
*/
- public static <UserT, OutputT> TypedWrite<UserT, Void, OutputT> writeCustomType() {
- return AvroIO.<UserT, OutputT>defaultWriteBuilder().setGenericRecords(false).build();
+ public static <UserT, OutputT> TypedWrite<UserT, Void, OutputT> writeCustomType(
+ Class<OutputT> recordClass) {
Review comment:
This is a breaking change. The class needs to be propagated for proper factories instanciation.
--
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] RustedBones commented on a change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r771257284
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroCoder.java
##########
@@ -272,26 +263,8 @@ public Schema get() {
}
}
- /**
- * A {@link Serializable} object that lazily supplies a {@link ReflectData} built from the
- * appropriate {@link ClassLoader} for the type encoded by this {@link AvroCoder}.
- */
- private static class SerializableReflectDataSupplier
- implements Serializable, Supplier<ReflectData> {
-
- private final Class<?> clazz;
-
- private SerializableReflectDataSupplier(Class<?> clazz) {
- this.clazz = clazz;
- }
-
- @Override
- public ReflectData get() {
- ReflectData reflectData = new ReflectData(clazz.getClassLoader());
- reflectData.addLogicalTypeConversion(new JodaTimestampConversion());
- return reflectData;
- }
- }
+ private final AvroSource.DatumReaderFactory<T> readerFactory;
+ private final AvroSink.DatumWriterFactory<T> writerFactory;
Review comment:
Ideally I would have liked to have those as constructor parameter.
As explained in the PR comment, this causes problem with the [`AvroCoderCloudObjectTranslator`](https://github.com/apache/beam/blob/master/runners/google-cloud-dataflow-java/src/main/java/org/apache/beam/runners/dataflow/util/AvroCoderCloudObjectTranslator.java) which serializes the `AvroCoder` in a CloudObject (json). I don't know how to serialize the factories in that case
--
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] aaltay commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1022775434
What is the next step on this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r785007324
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/AvroRecordSchema.java
##########
@@ -32,12 +32,14 @@
* objects, creating a schema that matches that inferred by the AVRO libraries.
*/
@SuppressWarnings({
+ "nullness", // TODO(https://issues.apache.org/jira/browse/BEAM-10402)
Review comment:
Why? It would be best instead to not introduce nullness errors to this file.
--
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] RustedBones commented on a change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r778182570
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/coders/AvroCoder.java
##########
@@ -272,26 +263,8 @@ public Schema get() {
}
}
- /**
- * A {@link Serializable} object that lazily supplies a {@link ReflectData} built from the
- * appropriate {@link ClassLoader} for the type encoded by this {@link AvroCoder}.
- */
- private static class SerializableReflectDataSupplier
- implements Serializable, Supplier<ReflectData> {
-
- private final Class<?> clazz;
-
- private SerializableReflectDataSupplier(Class<?> clazz) {
- this.clazz = clazz;
- }
-
- @Override
- public ReflectData get() {
- ReflectData reflectData = new ReflectData(clazz.getClassLoader());
- reflectData.addLogicalTypeConversion(new JodaTimestampConversion());
- return reflectData;
- }
- }
+ private final AvroSource.DatumReaderFactory<T> readerFactory;
+ private final AvroSink.DatumWriterFactory<T> writerFactory;
Review comment:
Edit: I've added the factories in the constructor.
`AvroCoderCloudObjectTranslator` will not take in account custom factories in current versions
--
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] aromanenko-dev commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-997961567
Thanks! I'm a bit busy with other PRs for the moment but I'll try to take a look on this in the next days.
CC: @RyanSkraba @iemejia fyi, if you have some time, ptal too
--
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] RustedBones commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1007453667
There are no breaking changes to the `AvroCoder` directly.
I'd like to add the possibility to create coders using datum factories. Users could customize (de)serialization and enable more advanced avro features (eg. [BEAM-9144](https://issues.apache.org/jira/browse/BEAM-9144) to get reader and writer supporting both java.time and joda.time)
The breaking changes are:
- Some `AvroIO` API requiring to pass class type
- `AvroCoderCloudObjectTranslator` is not able so serialize custom factories
Maybe we should do as for the `AvroGenericCoder` and give specialized implementation for the `AvroSpecificCoder` and `AvroReflectCoder`. Change visibility of the `AvroCoder` so users can extend and create their own.
Only provide the `CloudObjectTranslator`, for `AvroGenericCoder`, `AvroSpecificCoder` and `AvroReflectCoder` and let users provider their own translator if they use a custom avro coder
--
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 change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r785076419
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/AvroRecordSchema.java
##########
@@ -32,12 +32,14 @@
* objects, creating a schema that matches that inferred by the AVRO libraries.
*/
@SuppressWarnings({
+ "nullness", // TODO(https://issues.apache.org/jira/browse/BEAM-10402)
"rawtypes" // TODO(https://issues.apache.org/jira/browse/BEAM-10556)
})
public class AvroRecordSchema extends GetterBasedSchemaProvider {
@Override
public <T> Schema schemaFor(TypeDescriptor<T> typeDescriptor) {
- return toBeamSchema(ReflectData.get().getSchema(typeDescriptor.getRawType()));
+ Class<?> avroType = typeDescriptor.getRawType();
Review comment:
```suggestion
Class<?> avroType = typeDescriptor.getRawType();
@SuppressWarnings("nullness") // new ReflectData(ClassLoader) is not annotated to accept null
ClassLoader classLoader = avroType.getClassLoader
return toBeamSchema(new ReflectData(classLoader).getSchema(avroType));
```
--
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] aromanenko-dev commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1063048774
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] RustedBones commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1004199364
@aromanenko-dev yes, this is related to my changes. It has some impact on the [readAvrosWithBeamSchema](https://github.com/apache/beam/blob/a4e112974d8f3f51e646d9e714a4f8d456d69b4d/sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/pubsub/PubsubIO.java#L641).
To me, the only way to properly enable logical-type support with avro reflect classes is to create an `AvroCoder` with custom factories
--
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] RustedBones commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1007453667
There are no breaking changes to the `AvroCoder` directly.
I'd like to add the possibility to create coders using datum factories. Users could customize (de)serialization and enable more advanced avro features (eg. [BEAM-9144](https://issues.apache.org/jira/browse/BEAM-9144) to get reader and writer supporting both java.time and joda.time)
The breaking changes are:
- Some `AvroIO` API requiring to pass class type
- `AvroCoderCloudObjectTranslator` is not able so serialize custom factories
Maybe we should do as for the `AvroGenericCoder` and give specialized implementation for the `AvroSpecificCoder` and `AvroReflectCoder`. Change visibility of the `AvroCoder` so users can extend and create their own.
Only provide the `CloudObjectTranslator`, for `AvroGenericCoder`, `AvroSpecificCoder` and `AvroReflectCoder` and let users provider their own translator if they use a custom avro coder
--
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] RustedBones commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
RustedBones commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1057161168
Any plans on how to proceed with 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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] aaltay commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
aaltay commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1057269183
pinging @kennknowles for the above question.
--
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] aromanenko-dev removed a comment on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
aromanenko-dev removed a comment on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1059409659
--
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] TheNeuralBit commented on pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #16271:
URL: https://github.com/apache/beam/pull/16271#issuecomment-1005289411
Is it making breaking changes to `AvroCoder`?
--
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 change in pull request #16271: [BEAM-8388] Make sdk-java-core compatible with avro 1.8 & 1.9
Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #16271:
URL: https://github.com/apache/beam/pull/16271#discussion_r785075074
##########
File path: sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/AvroRecordSchema.java
##########
@@ -32,12 +32,14 @@
* objects, creating a schema that matches that inferred by the AVRO libraries.
*/
@SuppressWarnings({
+ "nullness", // TODO(https://issues.apache.org/jira/browse/BEAM-10402)
Review comment:
Makes sense. At some point we should improve upstream apache/avro or possibly just add a stub file here. For now, can you narrow the scope of the `@SuppressWarnings` annotation to this method? Or, ideally, to a single assignment statement like so:
```java
Class<?> avroType = typeDescriptor.getRawType();
@SuppressWarnings("nullness") // new ReflectData(ClassLoader) is not annotated to accept null
ClassLoader classLoader = avroType.getClassLoader
return toBeamSchema(new ReflectData(classLoader).getSchema(avroType));
```
--
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