-
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
base: main
Are you sure you want to change the base?
Changes from all commits
adfe079
cad2925
e30bdab
56fdf51
32b8933
5442086
30e3872
b9831b6
7ed2da4
32bee92
cda7368
e9203b8
9203077
eef5ab5
475183b
c32e3d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,6 +21,35 @@ public Version getVersion() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public abstract Optional<AdvancedExtension> getAdvancedExtension(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public abstract ExecutionBehavior getExecutionBehavior(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Validates that the execution behavior is properly configured. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * <p>This validation method ensures that: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * <ul> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * <li>The {@link ExecutionBehavior} field is present (not null or empty) - ExecutionBehavior is | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * a required field | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * <li>The {@link ExecutionBehavior.VariableEvaluationMode} is set to a valid value (not {@link | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_UNSPECIFIED}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * </ul> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @throws IllegalArgumentException if the execution behavior is not present, or if the variable | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * evaluation mode is set to {@link | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_UNSPECIFIED} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Value.Check | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| protected void check() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ExecutionBehavior behavior = getExecutionBehavior(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (behavior.getVariableEvaluationMode() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| == ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_UNSPECIFIED) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new IllegalArgumentException( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "ExecutionBehavior requires a specified VariableEvaluationMode, but got: " | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| + behavior.getVariableEvaluationMode()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public static ImmutablePlan.Builder builder() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ImmutablePlan.builder(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -69,4 +98,19 @@ public static ImmutableRoot.Builder builder() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ImmutableRoot.builder(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @Value.Immutable | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public abstract static class ExecutionBehavior { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public abstract VariableEvaluationMode getVariableEvaluationMode(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public static ImmutableExecutionBehavior.Builder builder() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ImmutableExecutionBehavior.builder(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public enum VariableEvaluationMode { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| VARIABLE_EVALUATION_MODE_UNSPECIFIED, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| VARIABLE_EVALUATION_MODE_PER_PLAN, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| VARIABLE_EVALUATION_MODE_PER_RECORD | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. There's an established pattern for representing these enums in the library that we should follow here for consistency: substrait-java/core/src/main/java/io/substrait/expression/Expression.java Lines 1758 to 1771 in bb8d922
substrait-java/core/src/main/java/io/substrait/expression/Expression.java Lines 2161 to 2174 in bb8d922
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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,87 @@ 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.VARIABLE_EVALUATION_MODE_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 | ||
| .VARIABLE_EVALUATION_MODE_UNSPECIFIED; | ||
| case VARIABLE_EVALUATION_MODE_PER_PLAN: | ||
| return io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode | ||
| .VARIABLE_EVALUATION_MODE_PER_PLAN; | ||
| case VARIABLE_EVALUATION_MODE_PER_RECORD: | ||
| return io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode | ||
| .VARIABLE_EVALUATION_MODE_PER_RECORD; | ||
| case UNRECOGNIZED: | ||
| default: | ||
| throw new IllegalArgumentException("Unknown VariableEvaluationMode: " + mode); | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.