Replace Class with TypeDescriptor in SchemaProvider implementations#31785
Replace Class with TypeDescriptor in SchemaProvider implementations#31785chamikaramj merged 11 commits intoapache:masterfrom
Conversation
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
|
Run Java PreCommit |
|
assign set of reviewers |
|
Assigning reviewers. If you would like to opt out of this review, comment R: @m-trieu for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Reminder, please take a look at this pr: @m-trieu @chamikaramj |
|
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment R: @damondouglas for label java. Available commands:
|
m-trieu
left a comment
m-trieu
left a comment
There was a problem hiding this comment.
LGTM for java, i would get an LGTM from someone more familiar with IOs as well.
chamikaramj
left a comment
chamikaramj
left a comment
There was a problem hiding this comment.
Sorry about the delay.
LGTM for I/O updates other than one comment. Thanks.
| @SuppressWarnings("rawtypes") | ||
| @Override | ||
| public List<FieldValueGetter> fieldValueGetters(Class<?> clazz, Schema schema) { | ||
| public List<FieldValueGetter> fieldValueGetters( |
There was a problem hiding this comment.
Noticed that we break backwards compatibility of public fields here but probably this is OK since these are expected be util classes intended only internal usage ?
There was a problem hiding this comment.
good point - this is the consequence of changing the methods' signatures in the GetterBasedSchemaProvider - it's not annotated with @Internal so maybe we should actually remain careful with that interface - maybe I'll add some default implementations of the backward compatible methods to that class
Restored and deprecated the old |
|
Run Java_GCP_IO_Direct PreCommit |
|
Run Java PreCommit |
|
+1 for deprecating the old public class instead of just changing the signature. LGTM |
…pache#31785) * refactor GetterBasedSchemaProvider's methods' signatures * update StaticSchemaInference's methods' signatures * update invocations of schemaFromClass * update FieldValueTypeSupplier's methods' signatures * replace ClassWithSchema with TypeDescriptorWithSchema as caching key * remove the stopgap default method in Factory interface * use TypeDescriptors in AutoValueUtils' methods * convert the class to generic type descriptor in DefaultSchema * deprecate GetterBasedSchemaProvider * update GetterBasedSchemaProviderV2 javadoc * delegate the deprecated methods instead of throwing
3 participants
This is the first part of a series of PRs which when brought together are equivalent to the change previously proposed in #31648 - the code of this PR doesn't introduce or remove any features from the schema inference mechanisms existing already, it merely prepares the ground for doing that in the subsequent PRs. It does so by passing on down the call hierarchy the
TypeDescriptors where previously the code worked with bareClasses. This way the full information carried byTypeDescriptors regarding the type signatures is not lost, and the schema inference, as well as, on the fly bytecode generation facilities will be able to use it to infer beam schemas for generic java types.Adding the support for schema inference of generic types was proposed in a section of this design doc.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
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, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
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.