You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "ahmedabu98 (via GitHub)" <gi...@apache.org> on 2024/06/10 18:56:50 UTC
[PR] Default translation for SchemaTransforms [beam]
ahmedabu98 opened a new pull request, #31558:
URL: https://github.com/apache/beam/pull/31558
(no comment)
--
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] Default translation for SchemaTransforms [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31558:
URL: https://github.com/apache/beam/pull/31558#issuecomment-2308381620
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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] Default translation for SchemaTransforms [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31558:
URL: https://github.com/apache/beam/pull/31558#discussion_r1633808204
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/transforms/SchemaTransform.java:
##########
@@ -33,5 +38,60 @@
* compatibility guarantees and it should not be implemented outside of the Beam repository.
*/
@Internal
-public abstract class SchemaTransform
- extends PTransform<PCollectionRowTuple, PCollectionRowTuple> {}
+public abstract class SchemaTransform extends PTransform<PCollectionRowTuple, PCollectionRowTuple> {
+ private @Nullable Row configurationRow;
+ private @Nullable String identifier;
+ private boolean registered = false;
Review Comment:
I use "registered" throughout, but I'm not sure if it's the best terminology to use in this context. Open to other suggestions!
--
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] Default translation for SchemaTransforms [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31558:
URL: https://github.com/apache/beam/pull/31558#issuecomment-2294849008
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions.
--
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] Default translation for SchemaTransforms [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #31558:
URL: https://github.com/apache/beam/pull/31558#issuecomment-2173835219
Yep that’s the main problem being solved here. To be clear though, transforms that come from *any* `SchemaTransformProvider` (doesn’t have to be `[Typed]`) already have an identifier and configRow (implicitly) attached. This PR makes this attachment explicit and available for translation.
> In this case, it's a bit odd because it seems we can only derive the inverse for the specific instances that we created, not generally for all instances of that class
By design, these SchemaTransforms are always created using the corresponding `SchemaTransformProvider.from(configRow)`. We never instantiate the SchemaTransform directly (there are no public SchemaTransform implementations). So we can safely assume any SchemaTransform instantiation will have come from a SchemaTransformProvider that we created.
I share the aversion towards adding unnecessary things to SchemaTransform though. I’ve been trying to cut things down as I go, but there’s probably still a better solution.
> Might be cleaner in that case to just have a separate (weakref) mapping (registrar) from PTransform instances to their corresponding configs
Thanks! Will take a stab at 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
Re: [PR] Default translation for SchemaTransforms [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #31558:
URL: https://github.com/apache/beam/pull/31558#issuecomment-2159244429
Added a description. This is ready for review now
--
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] Default translation for SchemaTransforms [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31558:
URL: https://github.com/apache/beam/pull/31558#discussion_r1633808204
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/transforms/SchemaTransform.java:
##########
@@ -33,5 +38,60 @@
* compatibility guarantees and it should not be implemented outside of the Beam repository.
*/
@Internal
-public abstract class SchemaTransform
- extends PTransform<PCollectionRowTuple, PCollectionRowTuple> {}
+public abstract class SchemaTransform extends PTransform<PCollectionRowTuple, PCollectionRowTuple> {
+ private @Nullable Row configurationRow;
+ private @Nullable String identifier;
+ private boolean registered = false;
Review Comment:
I use "registered" throughout, but I'm not sure if it's the best terminology here. Open to other suggestions!
--
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] Default translation for SchemaTransforms [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #31558: Default translation for SchemaTransforms
URL: https://github.com/apache/beam/pull/31558
--
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] Default translation for SchemaTransforms [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on PR #31558:
URL: https://github.com/apache/beam/pull/31558#issuecomment-2159115792
Could you add a description of what is improved 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
Re: [PR] Default translation for SchemaTransforms [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on PR #31558:
URL: https://github.com/apache/beam/pull/31558#issuecomment-2166214597
Taking a step back, is the problem that you're trying to solve is that those transforms that come from a `[Typed]SchemaTransformProvider` we already have a `(identifier + configRow) -> [Schema]Transform` mapping, and we should be able to automatically derive the inverse `[Schema]Transform -> (identifier + configRow)` mapping for these transforms?
In this case, it's a bit odd because it seems we can only derive the inverse for the specific instances that we created, not generally for all instances of that class. (But maybe that's good enough sometimes?) Might be cleaner in that case to just have a separate (weakref) mapping (registrar) from PTransform instances to their corresponding configs rather add a register method and private members to `[Schema]Transform` itself.
Separately it'd be good to make defining this pair of mappings so easy that it's "just how users write PTransforms" (maybe working it into builder patterns or providing a good annotation or something like that) and we get their externalization and semantic graph representation for free. But this is likely a larger project.
--
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] Default translation for SchemaTransforms [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31558:
URL: https://github.com/apache/beam/pull/31558#issuecomment-2159248010
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control
--
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] Default translation for SchemaTransforms [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #31558:
URL: https://github.com/apache/beam/pull/31558#issuecomment-2159244803
R: @robertwb
R: @chamikaramj
R: @kennknowles
--
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] Default translation for SchemaTransforms [beam]
Posted by "robertwb (via GitHub)" <gi...@apache.org>.
robertwb commented on code in PR #31558:
URL: https://github.com/apache/beam/pull/31558#discussion_r1638507846
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/schemas/transforms/SchemaTransform.java:
##########
@@ -33,5 +36,39 @@
* compatibility guarantees and it should not be implemented outside of the Beam repository.
*/
@Internal
-public abstract class SchemaTransform
- extends PTransform<PCollectionRowTuple, PCollectionRowTuple> {}
+public abstract class SchemaTransform extends PTransform<PCollectionRowTuple, PCollectionRowTuple> {
+ private @Nullable Row configurationRow;
Review Comment:
It feels like putting these (optional) private variables as part of the base class violates separation of concerns.
--
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