You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "tilgalas (via GitHub)" <gi...@apache.org> on 2024/07/05 15:56:26 UTC
[PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
tilgalas opened a new pull request, #31785:
URL: https://github.com/apache/beam/pull/31785
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 `TypeDescriptor`s where previously the code worked with bare `Class`es. This way the full information carried by `TypeDescriptor`s 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](https://docs.google.com/document/d/1zSQ9cnqtVM8ttJEuHBDE6hw4qjUuJy1dpZWB6IBTuOs/edit#heading=h.efves32ipr7n).
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] 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.
- [ ] 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://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-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)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+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 or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31785:
URL: https://github.com/apache/beam/pull/31785#issuecomment-2239011305
Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @damondouglas for label java.
R: @ahmedabu98 for label io.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
--
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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on code in PR #31785:
URL: https://github.com/apache/beam/pull/31785#discussion_r1692640457
##########
sdks/java/io/amazon-web-services2/src/main/java/org/apache/beam/sdk/io/aws2/schemas/AwsSchemaProvider.java:
##########
@@ -75,9 +75,11 @@ public class AwsSchemaProvider extends GetterBasedSchemaProvider {
@SuppressWarnings("rawtypes")
@Override
- public List<FieldValueGetter> fieldValueGetters(Class<?> clazz, Schema schema) {
+ public List<FieldValueGetter> fieldValueGetters(
Review 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 ?
--
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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "tilgalas (via GitHub)" <gi...@apache.org>.
tilgalas commented on code in PR #31785:
URL: https://github.com/apache/beam/pull/31785#discussion_r1693004106
##########
sdks/java/io/amazon-web-services2/src/main/java/org/apache/beam/sdk/io/aws2/schemas/AwsSchemaProvider.java:
##########
@@ -75,9 +75,11 @@ public class AwsSchemaProvider extends GetterBasedSchemaProvider {
@SuppressWarnings("rawtypes")
@Override
- public List<FieldValueGetter> fieldValueGetters(Class<?> clazz, Schema schema) {
+ public List<FieldValueGetter> fieldValueGetters(
Review 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 which
--
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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31785:
URL: https://github.com/apache/beam/pull/31785#issuecomment-2230739799
Reminder, please take a look at this pr: @m-trieu @chamikaramj
--
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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on PR #31785:
URL: https://github.com/apache/beam/pull/31785#issuecomment-2258849169
+1 for deprecating the old public class instead of just changing the signature.
LGTM
--
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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "tilgalas (via GitHub)" <gi...@apache.org>.
tilgalas commented on PR #31785:
URL: https://github.com/apache/beam/pull/31785#issuecomment-2255966396
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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "tilgalas (via GitHub)" <gi...@apache.org>.
tilgalas commented on PR #31785:
URL: https://github.com/apache/beam/pull/31785#issuecomment-2211192142
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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj merged PR #31785:
URL: https://github.com/apache/beam/pull/31785
--
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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "tilgalas (via GitHub)" <gi...@apache.org>.
tilgalas commented on PR #31785:
URL: https://github.com/apache/beam/pull/31785#issuecomment-2214499284
assign set of reviewers
--
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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "tilgalas (via GitHub)" <gi...@apache.org>.
tilgalas commented on PR #31785:
URL: https://github.com/apache/beam/pull/31785#issuecomment-2253103976
> Sorry about the delay.
>
> LGTM for I/O updates other than one comment. Thanks.
Restored and deprecated the old `GetterBasedSchemaProvider` so that if anyone depends on it, it should still work, and changed the base class of known subclasses of this class to the newer "V2" version that uses `TypeDescriptors`
--
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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "tilgalas (via GitHub)" <gi...@apache.org>.
tilgalas commented on PR #31785:
URL: https://github.com/apache/beam/pull/31785#issuecomment-2255793379
Run Java_GCP_IO_Direct 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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31785:
URL: https://github.com/apache/beam/pull/31785#issuecomment-2214506099
Assigning reviewers. If you would like to opt out of this review, comment `assign to next reviewer`:
R: @m-trieu for label java.
R: @chamikaramj for label io.
Available commands:
- `stop reviewer notifications` - opt out of the automated review tooling
- `remind me after tests pass` - tag the comment author after tests pass
- `waiting on author` - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)
The PR bot will only process comments in the main thread (not review comments).
--
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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31785:
URL: https://github.com/apache/beam/pull/31785#issuecomment-2211174076
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
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
Re: [PR] Replace Class with TypeDescriptor in SchemaProvider implementations [beam]
Posted by "tilgalas (via GitHub)" <gi...@apache.org>.
tilgalas commented on code in PR #31785:
URL: https://github.com/apache/beam/pull/31785#discussion_r1693004106
##########
sdks/java/io/amazon-web-services2/src/main/java/org/apache/beam/sdk/io/aws2/schemas/AwsSchemaProvider.java:
##########
@@ -75,9 +75,11 @@ public class AwsSchemaProvider extends GetterBasedSchemaProvider {
@SuppressWarnings("rawtypes")
@Override
- public List<FieldValueGetter> fieldValueGetters(Class<?> clazz, Schema schema) {
+ public List<FieldValueGetter> fieldValueGetters(
Review 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
--
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