transformable IncrementalExecutionResult #3693
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jbellenger
commented
Aug 23, 2024
| @@ -52,6 +53,13 @@ public static Builder fromExecutionResult(ExecutionResult executionResult) { | |||
| return new Builder().from(executionResult); | |||
| } | |||
|
|
|||
| @Override | |||
| public IncrementalExecutionResult transform(Consumer<ExecutionResult.Builder<?>> builderConsumer) { | |||
There was a problem hiding this comment.
It would be nice if this Consumer was actually for type IncrementalExecutionResult.Builder, but changing it would either break the override or require adding more generics to ExecutionResult.
As written, the builder given to the consumer is castable to IncrementalExecutionResult.Builder, which is better than nothing. I'm open to feedback/ideas on how to get better typing.
There was a problem hiding this comment.
Good point.
That's a tricky problem though, and I can't think of a nice solution right now.
I'm happy to merge the PR as is if it unblocks you.
felipe-gdr
approved these changes
Aug 25, 2024
|
Hi @jbellenger thanks for the fix! Later today I will release v22.3 which will include your PR, in case you need a point version release for work. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Calling
IncrementalExecutionResult.transformwill return anExecutionResultImpl(rather than anotherIncrementalExecutionResult), which can cause incremental parts of the execution result to get lost.This makes it difficult to add
@defersupport to a system with existing instrumentations, which might modify an ExecutionResult without knowing that it needs to be handled differently if it is incremental.This PR overrides the implementation of
IncrementalExecutionResult.transformto ensure that the transformed result retains its incremental nature.