#34009 avro generic record to beam row conversion added support for a… #34024
Conversation
… for all logical types and conversions
|
Run Java PreCommit |
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
assign set of reviewers |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @robertwb for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
| * 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. | ||
| */ |
There was a problem hiding this comment.
we don't need to include a license header here since we're not distributing this file (and it is under the RAT exclusion)
buildSrc/build.gradle.kts
Outdated
| @@ -45,7 +45,7 @@ dependencies { | |||
| implementation("com.github.spotbugs.snom:spotbugs-gradle-plugin:5.0.14") | |||
|
|
|||
| runtimeOnly("com.google.protobuf:protobuf-gradle-plugin:0.8.13") // Enable proto code generation | |||
| runtimeOnly("com.github.davidmc24.gradle.plugin:gradle-avro-plugin:1.9.1") // Enable Avro code generation | |||
| runtimeOnly("com.github.davidmc24.gradle.plugin:gradle-avro-plugin:1.1.0") // Enable Avro code generation. Version 1.1.0 is the last supporting avro 1.10.2 | |||
There was a problem hiding this comment.
I'm not sure about downgrading this dependency. Why do we specifically need to target 1.1.0? It is a rather old dependency, and downgrading could introduce other problems
There was a problem hiding this comment.
restored 1.9.1. My thinking is that since the earliest tested avro version is 1.10.2 (see sdks/java/extensions/avro/build.gradle) , it would be good to use a version of the avro plugin that uses avro 1.10.2.
In any case, the whole plugin seems no longer maintained, so maybe it would be a good idea to remove this altogether and just use the avro-tools jar, like elsewhere. seems like this could be addressed in a separate task though
| GENERIC_DATA_WITH_DEFAULT_CONVERSIONS = new GenericData(); | ||
| addLogicalTypeConversions(GENERIC_DATA_WITH_DEFAULT_CONVERSIONS); | ||
| GENERIC_DATA_WITH_DEFAULT_CONVERSIONS.addLogicalTypeConversion( | ||
| new Conversions.DecimalConversion()); |
There was a problem hiding this comment.
Is there a reason not to just add this to addLogicalTypeConversion
There was a problem hiding this comment.
in addLogicalTypeConversions I see this comment:
// do not add DecimalConversion by default as schema must have extra 'scale' and 'precision'
// properties. avro reflect already handles BigDecimal as string with the 'java-class' property
so I suppose that's the reason. I see that this method is only used in org.apache.beam.sdk.extensions.avro.io.AvroDatumFactory.ReflectDatumFactory so that seems relevant
|
Adding some folks who may have some more context here: @aromanenko-dev @Abacn |
…underlying type instead of types.Alias (apache#33868)
* add endpoint type to WorkerMetadataResponse proto * add default value to endpoint_type
* Add UUID support in Spanner Schema * Add test
#34009 avro generic record to beam row conversion added support for all logical types and conversions
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.