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/05/31 22:34:24 UTC
[PR] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
ahmedabu98 opened a new pull request, #31470:
URL: https://github.com/apache/beam/pull/31470
Fixes #31375
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on code in PR #31470:
URL: https://github.com/apache/beam/pull/31470#discussion_r1624829545
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/PCollectionRowTuple.java:
##########
@@ -180,6 +181,22 @@ public PCollection<Row> get(String tag) {
return pcollection;
}
+ /**
+ * Like {@link #get(String)}, but is a convenience method to get a single output without providing
+ * a tag for that output. Use only when there is a single output.
+ *
+ * <p>Throws {@link IllegalStateException} if more than one output exists in the {@link
+ * PCollectionRowTuple}.
+ */
+ public PCollection<Row> getOutput() {
Review Comment:
May be call this `getOnlyPCollection()` since `PCollectionRowTuple` can be used as input or output.
##########
sdks/java/managed/src/main/java/org/apache/beam/sdk/managed/Managed.java:
##########
@@ -194,7 +199,28 @@ public PCollectionRowTuple expand(PCollectionRowTuple input) {
SchemaTransform underlyingTransform =
new ManagedSchemaTransformProvider(getSupportedIdentifiers()).from(managedConfig);
- return input.apply(underlyingTransform);
+ return inputTuple.apply(underlyingTransform);
+ }
+
+ @VisibleForTesting
+ static PCollectionRowTuple resolveInput(PInput input) {
Review Comment:
Will it be possible to introduce a similar simplification for output so that callers do not have to call `getOutput` when there's only one output `PCollection` ?
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #31470:
URL: https://github.com/apache/beam/pull/31470#issuecomment-2143072718
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on PR #31470:
URL: https://github.com/apache/beam/pull/31470#issuecomment-2143072002
R: @chamikaramj
R: @VeronicaWasson
R: @damccorm
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31470:
URL: https://github.com/apache/beam/pull/31470#discussion_r1624941076
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/PCollectionRowTuple.java:
##########
@@ -180,6 +181,22 @@ public PCollection<Row> get(String tag) {
return pcollection;
}
+ /**
+ * Like {@link #get(String)}, but is a convenience method to get a single output without providing
+ * a tag for that output. Use only when there is a single output.
+ *
+ * <p>Throws {@link IllegalStateException} if more than one output exists in the {@link
+ * PCollectionRowTuple}.
+ */
+ public PCollection<Row> getOutput() {
Review Comment:
Renamed to `getSinglePCollection()`
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "weifonghsia (via GitHub)" <gi...@apache.org>.
weifonghsia commented on code in PR #31470:
URL: https://github.com/apache/beam/pull/31470#discussion_r1628130272
##########
sdks/java/managed/src/main/java/org/apache/beam/sdk/managed/Managed.java:
##########
@@ -66,11 +72,9 @@
* <p>The file's path can be passed in to the Managed API like so:
*
* <pre>{@code
- * PCollectionRowTuple input = PCollectionRowTuple.of("input", pipeline.apply(Create.of(...)))
+ * PCollection<Row> inputRows = pipeline.apply(Create.of(...));
*
- * PCollectionRowTuple output = input.apply(
- * Managed.write(ICEBERG)
- * .withConfigUrl(<config path>));
+ * input.apply(Managed.write(ICEBERG).withConfigUrl(<config path>));
Review Comment:
I think the comment example should read inputRows in line 77 or input in line 75 - they're not matching atm
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on PR #31470:
URL: https://github.com/apache/beam/pull/31470#issuecomment-2145978336
cc: @weifonghsia for input on this and the Managed API in genera.
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31470:
URL: https://github.com/apache/beam/pull/31470#discussion_r1625082136
##########
sdks/java/managed/src/main/java/org/apache/beam/sdk/managed/Managed.java:
##########
@@ -194,7 +199,28 @@ public PCollectionRowTuple expand(PCollectionRowTuple input) {
SchemaTransform underlyingTransform =
new ManagedSchemaTransformProvider(getSupportedIdentifiers()).from(managedConfig);
- return input.apply(underlyingTransform);
+ return inputTuple.apply(underlyingTransform);
+ }
+
+ @VisibleForTesting
+ static PCollectionRowTuple resolveInput(PInput input) {
Review Comment:
PCollectionRowTuple already implements apply() for PTransform<PCollectionRowTuple, OutputT>:
https://github.com/apache/beam/blob/4ad7037b55f4b711a67f3f2170bc8661a9b799b5/sdks/java/core/src/main/java/org/apache/beam/sdk/values/PCollectionRowTuple.java#L196-L199
Java doesn't let us define the same method with a different generic (e.g. PTransform<PCollection<Row>, OutputT>) because they will ultimately have the same erasure.
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31470:
URL: https://github.com/apache/beam/pull/31470#discussion_r1628219510
##########
sdks/java/managed/src/main/java/org/apache/beam/sdk/managed/Managed.java:
##########
@@ -66,11 +72,9 @@
* <p>The file's path can be passed in to the Managed API like so:
*
* <pre>{@code
- * PCollectionRowTuple input = PCollectionRowTuple.of("input", pipeline.apply(Create.of(...)))
+ * PCollection<Row> inputRows = pipeline.apply(Create.of(...));
*
- * PCollectionRowTuple output = input.apply(
- * Managed.write(ICEBERG)
- * .withConfigUrl(<config path>));
+ * input.apply(Managed.write(ICEBERG).withConfigUrl(<config path>));
Review Comment:
Thanks for catching this! fixing in #31517, PTAL
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on code in PR #31470:
URL: https://github.com/apache/beam/pull/31470#discussion_r1626661530
##########
sdks/java/managed/src/main/java/org/apache/beam/sdk/managed/Managed.java:
##########
@@ -194,7 +199,28 @@ public PCollectionRowTuple expand(PCollectionRowTuple input) {
SchemaTransform underlyingTransform =
new ManagedSchemaTransformProvider(getSupportedIdentifiers()).from(managedConfig);
- return input.apply(underlyingTransform);
+ return inputTuple.apply(underlyingTransform);
+ }
+
+ @VisibleForTesting
+ static PCollectionRowTuple resolveInput(PInput input) {
Review Comment:
I think we should think of ways to get rid of the `getSinglePCollection()` call in the future but this is already a very good improvement.
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on PR #31470:
URL: https://github.com/apache/beam/pull/31470#issuecomment-2148503112
LGTM. Thanks.
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31470:
URL: https://github.com/apache/beam/pull/31470#discussion_r1624885356
##########
sdks/java/managed/src/main/java/org/apache/beam/sdk/managed/Managed.java:
##########
@@ -194,7 +199,28 @@ public PCollectionRowTuple expand(PCollectionRowTuple input) {
SchemaTransform underlyingTransform =
new ManagedSchemaTransformProvider(getSupportedIdentifiers()).from(managedConfig);
- return input.apply(underlyingTransform);
+ return inputTuple.apply(underlyingTransform);
+ }
+
+ @VisibleForTesting
+ static PCollectionRowTuple resolveInput(PInput input) {
Review Comment:
Will have to think about this one some more. From what I see, it's possible if ManagedTransform returns a `POutput`, but that may be harmful. POutput doesn't implement `apply()` so it would prevent users from chaining downstream transforms.
We could also create an abstraction between PCollectionRowTuple and PCollection<Row> but that feels a little messy
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 merged PR #31470:
URL: https://github.com/apache/beam/pull/31470
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "chamikaramj (via GitHub)" <gi...@apache.org>.
chamikaramj commented on code in PR #31470:
URL: https://github.com/apache/beam/pull/31470#discussion_r1624961403
##########
sdks/java/managed/src/main/java/org/apache/beam/sdk/managed/Managed.java:
##########
@@ -194,7 +199,28 @@ public PCollectionRowTuple expand(PCollectionRowTuple input) {
SchemaTransform underlyingTransform =
new ManagedSchemaTransformProvider(getSupportedIdentifiers()).from(managedConfig);
- return input.apply(underlyingTransform);
+ return inputTuple.apply(underlyingTransform);
+ }
+
+ @VisibleForTesting
+ static PCollectionRowTuple resolveInput(PInput input) {
Review Comment:
May be we can implement `public <OutputT extends POutput> OutputT apply(PTransform)` for `PCollectionRowTuple` and error out if there's more than on `PCollection` ?
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "ahmedabu98 (via GitHub)" <gi...@apache.org>.
ahmedabu98 commented on code in PR #31470:
URL: https://github.com/apache/beam/pull/31470#discussion_r1624885356
##########
sdks/java/managed/src/main/java/org/apache/beam/sdk/managed/Managed.java:
##########
@@ -194,7 +199,28 @@ public PCollectionRowTuple expand(PCollectionRowTuple input) {
SchemaTransform underlyingTransform =
new ManagedSchemaTransformProvider(getSupportedIdentifiers()).from(managedConfig);
- return input.apply(underlyingTransform);
+ return inputTuple.apply(underlyingTransform);
+ }
+
+ @VisibleForTesting
+ static PCollectionRowTuple resolveInput(PInput input) {
Review Comment:
Will have to think about this one some more.
From what I see, it's possible if ManagedTransform returns a `POutput`, but that may be harmful. POutput doesn't implement `apply()` so it would prevent users from chaining downstream transforms.
We could also create an abstraction that encompasses PCollectionRowTuple and PCollection<Row> but that feels a little messy
--
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] Simplify Managed API to avoid dealing with PCollectionRowTuple [beam]
Posted by "weifonghsia (via GitHub)" <gi...@apache.org>.
weifonghsia commented on code in PR #31470:
URL: https://github.com/apache/beam/pull/31470#discussion_r1628165765
##########
sdks/java/managed/src/main/java/org/apache/beam/sdk/managed/Managed.java:
##########
@@ -66,11 +72,9 @@
* <p>The file's path can be passed in to the Managed API like so:
*
* <pre>{@code
- * PCollectionRowTuple input = PCollectionRowTuple.of("input", pipeline.apply(Create.of(...)))
+ * PCollection<Row> inputRows = pipeline.apply(Create.of(...));
*
- * PCollectionRowTuple output = input.apply(
- * Managed.write(ICEBERG)
- * .withConfigUrl(<config path>));
+ * input.apply(Managed.write(ICEBERG).withConfigUrl(<config path>));
Review Comment:
Otherwise, this LGTM - will really help simplify these deployments!
--
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