-
Notifications
You must be signed in to change notification settings - Fork 94
feat: add plan execution behavior #791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
nielspardon
wants to merge
19
commits into
substrait-io:main
Choose a base branch
from
nielspardon:par-execbehavior
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
adfe079
feat(core): add plan execution behavior
nielspardon cad2925
feat(isthmus): add plan execution behavior
nielspardon e30bdab
feat(spark): add plan execution behavior
nielspardon 56fdf51
docs: fix javadoc
nielspardon 32b8933
test(spark): fix testcase
nielspardon 5442086
feat: validate only when plan gets serialized to proto
nielspardon 30e3872
test: update tests
nielspardon b9831b6
chore: revert prior commits
nielspardon 7ed2da4
feat: set default for older plan versions
nielspardon 32bee92
docs: apply suggestions from code review
nielspardon cda7368
docs: update javadocs
nielspardon e9203b8
fix: revert "feat: set default for older plan versions"
nielspardon 9203077
fix: always set default execution behavior when absent
nielspardon eef5ab5
style: fix spotless
nielspardon 475183b
fix: address feedback from vbarua
nielspardon c32e3d0
docs: fix javadoc typo
nielspardon 60baaa7
fix: add enum proto mapping into enum
nielspardon 2e26e83
fix: scala and tests
nielspardon 702fde4
test: consolidate testcases
nielspardon File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,8 @@ | |
| import io.substrait.extension.ImmutableExtensionLookup; | ||
| import io.substrait.extension.ProtoExtensionConverter; | ||
| import io.substrait.extension.SimpleExtension.ExtensionCollection; | ||
| import io.substrait.plan.Plan.ExecutionBehavior; | ||
| import io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode; | ||
| import io.substrait.proto.PlanRel; | ||
| import io.substrait.relation.ProtoRelConverter; | ||
| import io.substrait.relation.Rel; | ||
|
|
@@ -66,6 +68,21 @@ protected ProtoRelConverter getProtoRelConverter(final ExtensionLookup functionL | |
| return new ProtoRelConverter(functionLookup, this.extensionCollection, protoExtensionConverter); | ||
| } | ||
|
|
||
| /** | ||
|
nielspardon marked this conversation as resolved.
|
||
| * Converts a protobuf {@link io.substrait.proto.Plan} to a {@link Plan} POJO. | ||
| * | ||
| * <p><b>Note:</b> Execution behavior is optional in the protobuf message, but the {@link Plan} | ||
| * POJO requires it. Conversion handles a missing execution behavior based on the plan version: | ||
| * | ||
| * <p><b>Note:</b> If {@code Plan.ExecutionBehavior.VariableEvaluationMode} is not set, it will be | ||
| * defaulted to {@code VARIABLE_EVALUATION_MODE_PER_PLAN}. Once other producers populate this | ||
| * field correctly, this compatibility workaround will be removed. | ||
| * | ||
| * @param plan the protobuf Plan to convert, must not be null | ||
| * @return the converted Plan POJO | ||
| * @throws IllegalArgumentException if the plan contains invalid data or if the execution behavior | ||
| * validation fails | ||
| */ | ||
| public Plan from(io.substrait.proto.Plan plan) { | ||
| ExtensionLookup functionLookup = ImmutableExtensionLookup.builder().from(plan).build(); | ||
| ProtoRelConverter relConverter = getProtoRelConverter(functionLookup); | ||
|
|
@@ -92,15 +109,84 @@ public Plan from(io.substrait.proto.Plan plan) { | |
| versionBuilder.producer(Optional.of(plan.getVersion().getProducer())); | ||
| } | ||
|
|
||
| return Plan.builder() | ||
| .roots(roots) | ||
| .expectedTypeUrls(plan.getExpectedTypeUrlsList()) | ||
| .advancedExtension( | ||
| Optional.ofNullable( | ||
| plan.hasAdvancedExtensions() | ||
| ? protoExtensionConverter.fromProto(plan.getAdvancedExtensions()) | ||
| : null)) | ||
| .version(versionBuilder.build()) | ||
| ImmutablePlan.Builder planBuilder = | ||
| Plan.builder() | ||
| .roots(roots) | ||
| .expectedTypeUrls(plan.getExpectedTypeUrlsList()) | ||
| .advancedExtension( | ||
| Optional.ofNullable( | ||
| plan.hasAdvancedExtensions() | ||
| ? protoExtensionConverter.fromProto(plan.getAdvancedExtensions()) | ||
| : null)) | ||
| .version(versionBuilder.build()); | ||
|
|
||
| // Set execution behavior (required field) | ||
| if (plan.hasExecutionBehavior()) { | ||
| planBuilder.executionBehavior(fromProtoExecutionBehavior(plan.getExecutionBehavior())); | ||
| } else { | ||
| // Set default ExecutionBehavior for older plans that don't have it | ||
| planBuilder.executionBehavior( | ||
| ExecutionBehavior.builder() | ||
| .variableEvaluationMode(VariableEvaluationMode.PER_PLAN) | ||
| .build()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually the only piece we need for backwards compatability IMO. |
||
| } | ||
|
|
||
| return planBuilder.build(); | ||
| } | ||
|
|
||
| /** | ||
| * Converts a protobuf {@link io.substrait.proto.ExecutionBehavior} to its POJO representation. | ||
| * | ||
| * <p>This method converts the execution behavior configuration from the protobuf format to the | ||
| * POJO representation, including the variable evaluation mode. | ||
| * | ||
| * @param executionBehavior the protobuf ExecutionBehavior to convert, must not be null | ||
| * @return the POJO ExecutionBehavior representation | ||
| * @throws IllegalArgumentException if the variable evaluation mode is unknown or UNRECOGNIZED | ||
| */ | ||
| private io.substrait.plan.Plan.ExecutionBehavior fromProtoExecutionBehavior( | ||
| final io.substrait.proto.ExecutionBehavior executionBehavior) { | ||
| return io.substrait.plan.Plan.ExecutionBehavior.builder() | ||
| .variableEvaluationMode( | ||
| fromProtoVariableEvaluationMode(executionBehavior.getVariableEvalMode())) | ||
| .build(); | ||
| } | ||
|
|
||
| /** | ||
| * Converts a protobuf {@link io.substrait.proto.ExecutionBehavior.VariableEvaluationMode} to its | ||
| * POJO representation. | ||
| * | ||
| * <p>Supported modes: | ||
| * | ||
| * <ul> | ||
| * <li>{@link | ||
| * io.substrait.proto.ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_UNSPECIFIED} | ||
| * - Unspecified mode (will cause validation failure in Plan) | ||
| * <li>{@link | ||
| * io.substrait.proto.ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_PER_PLAN} | ||
| * - Variables are evaluated once per plan execution | ||
| * <li>{@link | ||
| * io.substrait.proto.ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_PER_RECORD} | ||
| * - Variables are evaluated for each record | ||
| * </ul> | ||
| * | ||
| * @param mode the protobuf VariableEvaluationMode to convert, must not be null | ||
| * @return the POJO VariableEvaluationMode representation | ||
| * @throws IllegalArgumentException if the mode is UNRECOGNIZED or not supported | ||
| */ | ||
| private io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode | ||
| fromProtoVariableEvaluationMode( | ||
| final io.substrait.proto.ExecutionBehavior.VariableEvaluationMode mode) { | ||
| switch (mode) { | ||
| case VARIABLE_EVALUATION_MODE_UNSPECIFIED: | ||
| return io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode.UNSPECIFIED; | ||
| case VARIABLE_EVALUATION_MODE_PER_PLAN: | ||
| return io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN; | ||
| case VARIABLE_EVALUATION_MODE_PER_RECORD: | ||
| return io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode.PER_RECORD; | ||
| case UNRECOGNIZED: | ||
| default: | ||
| throw new IllegalArgumentException("Unknown VariableEvaluationMode: " + mode); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.