lukecwik commented on a change in pull request #12016:
URL: https://github.com/apache/beam/pull/12016#discussion_r448725242
##########
File path:
sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/DoFnInvokersTest.java
##########
@@ -847,44 +848,26 @@ public Object restriction() {
}
});
assertThat(tracker, instanceOf(BoundedDefaultTracker.class));
- invoker.invokeTruncateRestriction(
- new FakeArgumentProvider<String, String>() {
- @Override
- public RestrictionTracker restrictionTracker() {
- return tracker;
- }
-
- @Override
- public String element(DoFn<String, String> doFn) {
- return "blah";
- }
-
- @Override
- public Object restriction() {
- return "foo";
- }
-
- @Override
- public OutputReceiver<String> outputReceiver(DoFn<String, String>
doFn) {
- return new DoFn.OutputReceiver<String>() {
- private boolean invoked;
+ Optional result =
Review comment:
```suggestion
Optional<?> result =
```
##########
File path:
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/DoFnSignatures.java
##########
@@ -1830,13 +1828,13 @@ private static boolean hasAnnotation(Class<?>
annotation, List<Annotation> annot
TypeDescriptor<? extends DoFn<?, ?>> fnT,
Method m,
TypeDescriptor<?> inputT,
- TypeDescriptor<?> outputT,
TypeDescriptor<?> restrictionT,
FnAnalysisContext fnContext) {
// Method is of the form:
// @TruncateRestriction
- // void truncateRestriction(... parameters ...);
- errors.checkArgument(void.class.equals(m.getReturnType()), "Must return
void");
+ // Optional<RestrictionT> truncateRestriction(... parameters ...);
+ errors.checkArgument(
+ Optional.class.equals(m.getReturnType()), "Must return
Optional<Restriction>");
Review comment:
We should be able to validate that the return type is
Optional<RestrictionT>
##########
File path:
sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/reflect/DoFnInvokersTest.java
##########
@@ -944,42 +927,25 @@ public Object restriction() {
}
});
assertThat(tracker, instanceOf(UnboundedDefaultTracker.class));
- invoker.invokeTruncateRestriction(
- new FakeArgumentProvider<String, String>() {
- @Override
- public RestrictionTracker restrictionTracker() {
- return tracker;
- }
-
- @Override
- public String element(DoFn<String, String> doFn) {
- return "blah";
- }
-
- @Override
- public Object restriction() {
- return "foo";
- }
-
- @Override
- public OutputReceiver<String> outputReceiver(DoFn<String, String>
doFn) {
- return new DoFn.OutputReceiver<String>() {
- private final boolean shouldInvoked = false;
+ Optional result =
Review comment:
```suggestion
Optional<?> result =
```
##########
File path:
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/reflect/DoFnInvoker.java
##########
@@ -95,7 +96,8 @@ void invokeOnTimer(
void invokeSplitRestriction(ArgumentProvider<InputT, OutputT> arguments);
/** Invoke the {@link TruncateRestriction} method on the bound {@link DoFn}.
*/
- void invokeTruncateRestriction(ArgumentProvider<InputT, OutputT> arguments);
+ <RestrictionT> Optional<RestrictionT> invokeTruncateRestriction(
Review comment:
The reason why I though it would make sense to return Optional was that
I was hoping that it would support the case when RestrictionT being null is a
valid use case.
This doesn't turn out to work with Optional since it doesn't support null
values. Would you rather have this API return null like Python returning None
for consistency? (null meaning that there is nothing meaningful to process)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
