From adfe07968c5cca75df772e1deab5e9c96b9272db Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Mon, 30 Mar 2026 11:20:55 +0200 Subject: [PATCH 01/19] feat(core): add plan execution behavior BREAKING CHANGES: execution behavior is a required field introduced in Substrait v0.87.0 Signed-off-by: Niels Pardon --- .../io/substrait/dsl/SubstraitBuilder.java | 47 ++- .../src/main/java/io/substrait/plan/Plan.java | 47 +++ .../io/substrait/plan/PlanProtoConverter.java | 81 ++++ .../io/substrait/plan/ProtoPlanConverter.java | 107 +++++- .../io/substrait/plan/PlanConverterTest.java | 353 +++++++++++++++++- .../io/substrait/plan/PlanValidationTest.java | 120 ++++++ .../plan-roundtrip/complex-expected-plan.json | 5 +- .../plan-roundtrip/complex-input-plan.json | 5 +- .../plan-roundtrip/simple-expected-plan.json | 5 +- .../plan-roundtrip/simple-input-plan.json | 5 +- .../zero-anchor-expected-plan.json | 5 +- .../zero-anchor-input-plan.json | 5 +- 12 files changed, 759 insertions(+), 26 deletions(-) create mode 100644 core/src/test/java/io/substrait/plan/PlanValidationTest.java diff --git a/core/src/main/java/io/substrait/dsl/SubstraitBuilder.java b/core/src/main/java/io/substrait/dsl/SubstraitBuilder.java index 54d4f63a3..fc06c4932 100644 --- a/core/src/main/java/io/substrait/dsl/SubstraitBuilder.java +++ b/core/src/main/java/io/substrait/dsl/SubstraitBuilder.java @@ -16,7 +16,9 @@ import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.SimpleExtension; import io.substrait.function.ToTypeString; +import io.substrait.plan.ImmutableExecutionBehavior; import io.substrait.plan.Plan; +import io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode; import io.substrait.relation.AbstractWriteRel; import io.substrait.relation.Aggregate; import io.substrait.relation.Aggregate.Measure; @@ -1540,13 +1542,54 @@ public Plan.Root root(Rel rel) { } /** - * Creates a plan from a plan root. + * Creates a plan from a plan root with default execution behavior. + * + *

The plan is created with {@link VariableEvaluationMode#VARIABLE_EVALUATION_MODE_PER_PLAN} as + * the default variable evaluation mode. To specify a custom execution behavior, use {@link + * #plan(Plan.ExecutionBehavior, Plan.Root)} instead. * * @param root the plan root * @return a new {@link Plan} */ public Plan plan(Plan.Root root) { - return Plan.builder().addRoots(root).build(); + return plan( + ImmutableExecutionBehavior.builder() + .variableEvaluationMode(VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .build(), + root); + } + + /** + * Creates a plan from a plan root with custom execution behavior. + * + * @param executionBehavior the execution behavior for the plan + * @param root the plan root + * @return a new {@link Plan} + */ + public Plan plan(Plan.ExecutionBehavior executionBehavior, Plan.Root root) { + return Plan.builder().executionBehavior(executionBehavior).addRoots(root).build(); + } + + /** + * Creates a plan from multiple plan roots with custom execution behavior. + * + * @param executionBehavior the execution behavior for the plan + * @param roots the plan roots + * @return a new {@link Plan} + */ + public Plan plan(Plan.ExecutionBehavior executionBehavior, Plan.Root... roots) { + return Plan.builder().executionBehavior(executionBehavior).roots(Arrays.asList(roots)).build(); + } + + /** + * Creates a plan from multiple plan roots with custom execution behavior. + * + * @param executionBehavior the execution behavior for the plan + * @param roots the plan roots as an iterable + * @return a new {@link Plan} + */ + public Plan plan(Plan.ExecutionBehavior executionBehavior, Iterable roots) { + return Plan.builder().executionBehavior(executionBehavior).roots(roots).build(); } /** diff --git a/core/src/main/java/io/substrait/plan/Plan.java b/core/src/main/java/io/substrait/plan/Plan.java index 6436993f4..a3f897632 100644 --- a/core/src/main/java/io/substrait/plan/Plan.java +++ b/core/src/main/java/io/substrait/plan/Plan.java @@ -21,6 +21,38 @@ public Version getVersion() { public abstract Optional getAdvancedExtension(); + public abstract Optional getExecutionBehavior(); + + /** + * Validates that the execution behavior is properly configured. + * + *

This validation method ensures that: + * + *

+ * + * @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() { + if (!getExecutionBehavior().isPresent()) { + throw new IllegalArgumentException("ExecutionBehavior is required but was not set"); + } + ExecutionBehavior behavior = getExecutionBehavior().get(); + 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 +101,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 + } + } } diff --git a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java index 03b2a4227..d463999a8 100644 --- a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java +++ b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java @@ -4,6 +4,8 @@ import io.substrait.extension.ExtensionCollector; import io.substrait.extension.ExtensionProtoConverter; import io.substrait.extension.SimpleExtension.ExtensionCollection; +import io.substrait.proto.ExecutionBehavior; +import io.substrait.proto.ExecutionBehavior.VariableEvaluationMode; import io.substrait.proto.Plan; import io.substrait.proto.PlanRel; import io.substrait.proto.Rel; @@ -61,6 +63,26 @@ public PlanProtoConverter( this.extensionProtoConverter = extensionProtoConverter; } + /** + * Converts a {@link io.substrait.plan.Plan} object to its protobuf representation. + * + *

This method performs a complete conversion of the Plan object, including: + * + *

+ * + *

The execution behavior is optional. If present, it will be converted using {@link + * #toProtoExecutionBehavior(io.substrait.plan.Plan.ExecutionBehavior)}. + * + * @param plan the Plan object to convert, must not be null + * @return the protobuf Plan representation + * @throws IllegalArgumentException if the plan contains invalid data + */ public Plan toProto(final io.substrait.plan.Plan plan) { final List planRels = new ArrayList<>(); final ExtensionCollector functionCollector = new ExtensionCollector(extensionCollection); @@ -97,6 +119,65 @@ public Plan toProto(final io.substrait.plan.Plan plan) { builder.setVersion(versionBuilder); + // Set execution behavior if present + if (plan.getExecutionBehavior().isPresent()) { + builder.setExecutionBehavior(toProtoExecutionBehavior(plan.getExecutionBehavior().get())); + } + return builder.build(); } + + /** + * Converts an {@link io.substrait.plan.Plan.ExecutionBehavior} to its protobuf representation. + * + *

This method converts the execution behavior configuration, including the variable evaluation + * mode, from the POJO representation to the protobuf format. + * + * @param executionBehavior the ExecutionBehavior to convert, must not be null + * @return the protobuf ExecutionBehavior representation + * @throws IllegalArgumentException if the variable evaluation mode is unknown + */ + private ExecutionBehavior toProtoExecutionBehavior( + final io.substrait.plan.Plan.ExecutionBehavior executionBehavior) { + return ExecutionBehavior.newBuilder() + .setVariableEvalMode( + toProtoVariableEvaluationMode(executionBehavior.getVariableEvaluationMode())) + .build(); + } + + /** + * Converts a {@link io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode} to its + * protobuf representation. + * + *

Supported modes: + * + *

+ * + * @param mode the VariableEvaluationMode to convert, must not be null + * @return the protobuf VariableEvaluationMode representation + * @throws IllegalArgumentException if the mode is unknown or not supported + */ + private VariableEvaluationMode toProtoVariableEvaluationMode( + final io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode mode) { + switch (mode) { + case VARIABLE_EVALUATION_MODE_UNSPECIFIED: + return VariableEvaluationMode.VARIABLE_EVALUATION_MODE_UNSPECIFIED; + case VARIABLE_EVALUATION_MODE_PER_PLAN: + return VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN; + case VARIABLE_EVALUATION_MODE_PER_RECORD: + return VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD; + default: + throw new IllegalArgumentException("Unknown VariableEvaluationMode: " + mode); + } + } } diff --git a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java index 2e5334b3e..cb719b6c9 100644 --- a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java +++ b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java @@ -66,6 +66,29 @@ protected ProtoRelConverter getProtoRelConverter(final ExtensionLookup functionL return new ProtoRelConverter(functionLookup, this.extensionCollection, protoExtensionConverter); } + /** + * Converts a protobuf {@link io.substrait.proto.Plan} to a {@link Plan} POJO. + * + *

This method performs a complete conversion from the protobuf representation, including: + * + *

+ * + *

Note: While execution behavior is optional in the protobuf message, the Plan + * validation will still fail if ExecutionBehavior is not set or if it contains + * VARIABLE_EVALUATION_MODE_UNSPECIFIED. Ensure the protobuf Plan has a valid execution behavior + * before conversion. + * + * @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 +115,81 @@ 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 if present + if (plan.hasExecutionBehavior()) { + planBuilder.executionBehavior(fromProtoExecutionBehavior(plan.getExecutionBehavior())); + } + + return planBuilder.build(); + } + + /** + * Converts a protobuf {@link io.substrait.proto.ExecutionBehavior} to its POJO representation. + * + *

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. + * + *

Supported modes: + * + *

+ * + * @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); + } + } } diff --git a/core/src/test/java/io/substrait/plan/PlanConverterTest.java b/core/src/test/java/io/substrait/plan/PlanConverterTest.java index ab0a321b4..0c2a7f9ba 100644 --- a/core/src/test/java/io/substrait/plan/PlanConverterTest.java +++ b/core/src/test/java/io/substrait/plan/PlanConverterTest.java @@ -1,7 +1,11 @@ package io.substrait.plan; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import io.substrait.expression.Expression; import io.substrait.expression.ExpressionCreator; @@ -21,13 +25,25 @@ import org.junit.jupiter.api.Test; class PlanConverterTest { + private final PlanProtoConverter toProtoConverter = new PlanProtoConverter(); + private final ProtoPlanConverter fromProtoConverter = new ProtoPlanConverter(); + + private static Plan.ExecutionBehavior defaultExecutionBehavior() { + return ImmutableExecutionBehavior.builder() + .variableEvaluationMode( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .build(); + } + @Test void emptyAdvancedExtensionTest() { - final Plan plan = Plan.builder().advancedExtension(AdvancedExtension.builder().build()).build(); - final PlanProtoConverter toProtoConverter = new PlanProtoConverter(); + final Plan plan = + Plan.builder() + .executionBehavior(defaultExecutionBehavior()) + .advancedExtension(AdvancedExtension.builder().build()) + .build(); final io.substrait.proto.Plan protoPlan = toProtoConverter.toProto(plan); - final ProtoPlanConverter fromProtoConverter = new ProtoPlanConverter(); final Plan plan2 = fromProtoConverter.from(protoPlan); assertEquals(plan, plan2); @@ -39,9 +55,9 @@ void enhancementOnlyAdvancedExtensionWithoutExtensionProtoConverter() { final Plan plan = Plan.builder() + .executionBehavior(defaultExecutionBehavior()) .advancedExtension(AdvancedExtension.builder().enhancement(enhanced).build()) .build(); - final PlanProtoConverter toProtoConverter = new PlanProtoConverter(); assertThrows( UnsupportedOperationException.class, @@ -55,13 +71,13 @@ void enhancementOnlyAdvancedExtensionWithExtensionProtoConverter() { final Plan plan = Plan.builder() + .executionBehavior(defaultExecutionBehavior()) .advancedExtension(AdvancedExtension.builder().enhancement(enhanced).build()) .build(); final PlanProtoConverter toProtoConverter = new PlanProtoConverter(new StringHolderHandlingExtensionProtoConverter()); final io.substrait.proto.Plan protoPlan = toProtoConverter.toProto(plan); - final ProtoPlanConverter fromProtoConverter = new ProtoPlanConverter(); assertThrows( UnsupportedOperationException.class, () -> fromProtoConverter.from(protoPlan), @@ -74,6 +90,7 @@ void enhancementOnlyAdvancedExtensionWithExtensionProtoConverterAndProtoExtensio final Plan plan = Plan.builder() + .executionBehavior(defaultExecutionBehavior()) .advancedExtension(AdvancedExtension.builder().enhancement(enhanced).build()) .build(); final PlanProtoConverter toProtoConverter = @@ -93,9 +110,9 @@ void optimizationOnlyAdvancedExtensionWithoutExtensionProtoConverter() { final Plan plan = Plan.builder() + .executionBehavior(defaultExecutionBehavior()) .advancedExtension(AdvancedExtension.builder().addOptimizations(optimized).build()) .build(); - final PlanProtoConverter toProtoConverter = new PlanProtoConverter(); assertThrows( UnsupportedOperationException.class, @@ -109,13 +126,13 @@ void optimizationOnlyAdvancedExtensionWithExtensionProtoConverter() { final Plan plan = Plan.builder() + .executionBehavior(defaultExecutionBehavior()) .advancedExtension(AdvancedExtension.builder().addOptimizations(optimized).build()) .build(); final PlanProtoConverter toProtoConverter = new PlanProtoConverter(new StringHolderHandlingExtensionProtoConverter()); final io.substrait.proto.Plan protoPlan = toProtoConverter.toProto(plan); - final ProtoPlanConverter fromProtoConverter = new ProtoPlanConverter(); assertThrows( UnsupportedOperationException.class, () -> fromProtoConverter.from(protoPlan), @@ -128,6 +145,7 @@ void optimizationOnlyAdvancedExtensionWithExtensionProtoConverterAndProtoExtensi final Plan plan = Plan.builder() + .executionBehavior(defaultExecutionBehavior()) .advancedExtension(AdvancedExtension.builder().addOptimizations(optimized).build()) .build(); final PlanProtoConverter toProtoConverter = @@ -148,6 +166,7 @@ void advancedExtensionWithEnhancementAndOptimization() { final Plan plan = Plan.builder() + .executionBehavior(defaultExecutionBehavior()) .advancedExtension( AdvancedExtension.builder() .enhancement(enhanced) @@ -172,6 +191,7 @@ void planIncludingRelationWithAdvancedExtension() { final Plan plan = Plan.builder() + .executionBehavior(defaultExecutionBehavior()) .addRoots( Root.builder() .input( @@ -304,9 +324,12 @@ void nestedUserDefinedTypesShareExtensionCollector() { false, nullablePointLiteral, pointLiteral, vectorOfPointLiteral)) .build(); - Plan plan = Plan.builder().addRoots(Root.builder().input(virtualTable).build()).build(); + Plan plan = + Plan.builder() + .executionBehavior(defaultExecutionBehavior()) + .addRoots(Root.builder().input(virtualTable).build()) + .build(); - PlanProtoConverter toProtoConverter = new PlanProtoConverter(); io.substrait.proto.Plan protoPlan = toProtoConverter.toProto(plan); assertEquals(1, protoPlan.getExtensionUrnsCount(), "Should have exactly 1 extension URN"); @@ -319,4 +342,316 @@ void nestedUserDefinedTypesShareExtensionCollector() { Plan roundTrippedPlan = fromProtoConverter.from(protoPlan); assertEquals(plan, roundTrippedPlan, "Plan should roundtrip correctly"); } + + /** + * Conversion of Plan with VARIABLE_EVALUATION_MODE_PER_PLAN to protobuf. + * + *

Verifies that a Plan with ExecutionBehavior set to PER_PLAN mode is correctly converted to + * protobuf format, including the execution behavior field. + */ + @Test + void testToProtoWithExecutionBehaviorPerPlan() { + // Create a Plan with ExecutionBehavior set to PER_PLAN + Plan.ExecutionBehavior executionBehavior = + ImmutableExecutionBehavior.builder() + .variableEvaluationMode( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .build(); + + Plan plan = + ImmutablePlan.builder() + .executionBehavior(executionBehavior) + .roots(Collections.emptyList()) + .expectedTypeUrls(Collections.emptyList()) + .build(); + + // Convert to protobuf + io.substrait.proto.Plan protoPlan = toProtoConverter.toProto(plan); + + // Verify the protobuf has execution behavior + assertTrue(protoPlan.hasExecutionBehavior(), "Protobuf Plan should have ExecutionBehavior"); + assertEquals( + io.substrait.proto.ExecutionBehavior.VariableEvaluationMode + .VARIABLE_EVALUATION_MODE_PER_PLAN, + protoPlan.getExecutionBehavior().getVariableEvalMode(), + "Variable evaluation mode should be PER_PLAN"); + } + + /** + * Conversion of Plan with VARIABLE_EVALUATION_MODE_PER_RECORD to protobuf. + * + *

Verifies that a Plan with ExecutionBehavior set to PER_RECORD mode is correctly converted to + * protobuf format. + */ + @Test + void testToProtoWithExecutionBehaviorPerRecord() { + // Create a Plan with ExecutionBehavior set to PER_RECORD + Plan.ExecutionBehavior executionBehavior = + ImmutableExecutionBehavior.builder() + .variableEvaluationMode( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD) + .build(); + + Plan plan = + ImmutablePlan.builder() + .executionBehavior(executionBehavior) + .roots(Collections.emptyList()) + .expectedTypeUrls(Collections.emptyList()) + .build(); + + // Convert to protobuf + io.substrait.proto.Plan protoPlan = toProtoConverter.toProto(plan); + + // Verify the protobuf has execution behavior + assertTrue(protoPlan.hasExecutionBehavior(), "Protobuf Plan should have ExecutionBehavior"); + assertEquals( + io.substrait.proto.ExecutionBehavior.VariableEvaluationMode + .VARIABLE_EVALUATION_MODE_PER_RECORD, + protoPlan.getExecutionBehavior().getVariableEvalMode(), + "Variable evaluation mode should be PER_RECORD"); + } + + /** + * Conversion from protobuf Plan with ExecutionBehavior to POJO. + * + *

Verifies that a protobuf Plan with ExecutionBehavior is correctly converted to the POJO + * representation. + */ + @Test + void testFromProtoWithExecutionBehaviorPerPlan() { + // Create a protobuf Plan with ExecutionBehavior + io.substrait.proto.Plan protoPlan = + io.substrait.proto.Plan.newBuilder() + .setExecutionBehavior( + io.substrait.proto.ExecutionBehavior.newBuilder() + .setVariableEvalMode( + io.substrait.proto.ExecutionBehavior.VariableEvaluationMode + .VARIABLE_EVALUATION_MODE_PER_PLAN) + .build()) + .build(); + + // Convert to POJO + Plan plan = fromProtoConverter.from(protoPlan); + + // Verify the POJO has execution behavior + assertTrue( + plan.getExecutionBehavior().isPresent(), "Plan should have ExecutionBehavior present"); + assertEquals( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, + plan.getExecutionBehavior().get().getVariableEvaluationMode(), + "Variable evaluation mode should be PER_PLAN"); + } + + /** + * Conversion from protobuf Plan with PER_RECORD mode to POJO. + * + *

Verifies that a protobuf Plan with PER_RECORD execution behavior is correctly converted. + */ + @Test + void testFromProtoWithExecutionBehaviorPerRecord() { + // Create a protobuf Plan with ExecutionBehavior set to PER_RECORD + io.substrait.proto.Plan protoPlan = + io.substrait.proto.Plan.newBuilder() + .setExecutionBehavior( + io.substrait.proto.ExecutionBehavior.newBuilder() + .setVariableEvalMode( + io.substrait.proto.ExecutionBehavior.VariableEvaluationMode + .VARIABLE_EVALUATION_MODE_PER_RECORD) + .build()) + .build(); + + // Convert to POJO + Plan plan = fromProtoConverter.from(protoPlan); + + // Verify the POJO has execution behavior + assertTrue( + plan.getExecutionBehavior().isPresent(), "Plan should have ExecutionBehavior present"); + assertEquals( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD, + plan.getExecutionBehavior().get().getVariableEvaluationMode(), + "Variable evaluation mode should be PER_RECORD"); + } + + /** + * Conversion from protobuf Plan without ExecutionBehavior. + * + *

Verifies that a protobuf Plan without ExecutionBehavior results in a POJO Plan that fails + * validation (since ExecutionBehavior is required). + */ + @Test + void testFromProtoWithoutExecutionBehaviorFailsValidation() { + // Create a protobuf Plan without ExecutionBehavior + io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); + + // Attempt to convert to POJO - should fail validation + assertThrows( + IllegalArgumentException.class, + () -> fromProtoConverter.from(protoPlan), + "Conversion should fail when ExecutionBehavior is not set"); + } + + /** + * Test case 6: Conversion from protobuf Plan with UNSPECIFIED mode fails validation. + * + *

Verifies that a protobuf Plan with VARIABLE_EVALUATION_MODE_UNSPECIFIED results in a + * validation failure when converted to POJO. + */ + @Test + void testFromProtoWithUnspecifiedModeFailsValidation() { + // Create a protobuf Plan with UNSPECIFIED mode + io.substrait.proto.Plan protoPlan = + io.substrait.proto.Plan.newBuilder() + .setExecutionBehavior( + io.substrait.proto.ExecutionBehavior.newBuilder() + .setVariableEvalMode( + io.substrait.proto.ExecutionBehavior.VariableEvaluationMode + .VARIABLE_EVALUATION_MODE_UNSPECIFIED) + .build()) + .build(); + + // Attempt to convert to POJO - should fail validation + IllegalArgumentException exception = + assertThrows( + IllegalArgumentException.class, + () -> fromProtoConverter.from(protoPlan), + "Conversion should fail when VariableEvaluationMode is UNSPECIFIED"); + + assertTrue( + exception.getMessage().contains("VariableEvaluationMode"), + "Error message should mention VariableEvaluationMode"); + } + + /** + * Round-trip conversion with PER_PLAN mode. + * + *

Verifies that converting a Plan to protobuf and back preserves all data, including the + * execution behavior with PER_PLAN mode. + */ + @Test + void testRoundTripWithExecutionBehaviorPerPlan() { + // Create original Plan + Plan.ExecutionBehavior executionBehavior = + ImmutableExecutionBehavior.builder() + .variableEvaluationMode( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .build(); + + Plan originalPlan = + ImmutablePlan.builder() + .executionBehavior(executionBehavior) + .roots(Collections.emptyList()) + .expectedTypeUrls(Collections.emptyList()) + .build(); + + // Convert to protobuf and back + io.substrait.proto.Plan protoPlan = toProtoConverter.toProto(originalPlan); + Plan roundTrippedPlan = fromProtoConverter.from(protoPlan); + + // Verify data integrity + assertTrue( + roundTrippedPlan.getExecutionBehavior().isPresent(), + "Round-tripped Plan should have ExecutionBehavior"); + assertEquals( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, + roundTrippedPlan.getExecutionBehavior().get().getVariableEvaluationMode(), + "Variable evaluation mode should be preserved"); + assertEquals( + originalPlan.getRoots().size(), + roundTrippedPlan.getRoots().size(), + "Number of roots should be preserved"); + assertEquals( + originalPlan.getExpectedTypeUrls().size(), + roundTrippedPlan.getExpectedTypeUrls().size(), + "Number of expected type URLs should be preserved"); + } + + /** + * Round-trip conversion with PER_RECORD mode. + * + *

Verifies that converting a Plan with PER_RECORD mode to protobuf and back preserves the + * execution behavior correctly. + */ + @Test + void testRoundTripWithExecutionBehaviorPerRecord() { + // Create original Plan + Plan.ExecutionBehavior executionBehavior = + ImmutableExecutionBehavior.builder() + .variableEvaluationMode( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD) + .build(); + + Plan originalPlan = + ImmutablePlan.builder() + .executionBehavior(executionBehavior) + .roots(Collections.emptyList()) + .expectedTypeUrls(Collections.emptyList()) + .build(); + + // Convert to protobuf and back + io.substrait.proto.Plan protoPlan = toProtoConverter.toProto(originalPlan); + Plan roundTrippedPlan = fromProtoConverter.from(protoPlan); + + // Verify data integrity + assertTrue( + roundTrippedPlan.getExecutionBehavior().isPresent(), + "Round-tripped Plan should have ExecutionBehavior"); + assertEquals( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD, + roundTrippedPlan.getExecutionBehavior().get().getVariableEvaluationMode(), + "Variable evaluation mode should be preserved"); + } + + /** + * Empty plan with only execution behavior. + * + *

Verifies that a minimal Plan with only execution behavior (no roots, no type URLs) can be + * successfully converted. + */ + @Test + void testRoundTripEmptyPlanWithExecutionBehavior() { + // Create minimal Plan + Plan.ExecutionBehavior executionBehavior = + ImmutableExecutionBehavior.builder() + .variableEvaluationMode( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .build(); + + Plan originalPlan = + ImmutablePlan.builder() + .executionBehavior(executionBehavior) + .roots(Collections.emptyList()) + .expectedTypeUrls(Collections.emptyList()) + .build(); + + // Verify conversion succeeds + assertDoesNotThrow( + () -> { + io.substrait.proto.Plan protoPlan = toProtoConverter.toProto(originalPlan); + Plan roundTrippedPlan = fromProtoConverter.from(protoPlan); + assertNotNull(roundTrippedPlan, "Round-tripped Plan should not be null"); + }, + "Empty plan with execution behavior should convert successfully"); + } + + /** + * Verify that protobuf without execution behavior field is handled. + * + *

Tests the edge case where a protobuf Plan doesn't have the execution behavior field set at + * all. + */ + @Test + void testFromProtoMissingExecutionBehaviorField() { + // Create protobuf Plan without setting execution behavior + io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); + + // Verify hasExecutionBehavior returns false + assertFalse( + protoPlan.hasExecutionBehavior(), "Protobuf Plan should not have execution behavior field"); + + // Conversion should fail validation + assertThrows( + IllegalArgumentException.class, + () -> fromProtoConverter.from(protoPlan), + "Conversion should fail when execution behavior is missing"); + } } diff --git a/core/src/test/java/io/substrait/plan/PlanValidationTest.java b/core/src/test/java/io/substrait/plan/PlanValidationTest.java new file mode 100644 index 000000000..906074ed6 --- /dev/null +++ b/core/src/test/java/io/substrait/plan/PlanValidationTest.java @@ -0,0 +1,120 @@ +package io.substrait.plan; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.junit.jupiter.api.Test; + +/** + * Test cases for the Plan validation method that ensures ExecutionBehavior is properly configured. + */ +class PlanValidationTest { + + /** + * Test case 1: Valid execution behavior with proper variable evaluation mode. + * + *

This test verifies that a Plan with a properly configured ExecutionBehavior (with + * VariableEvaluationMode set to a valid value other than UNSPECIFIED) is successfully created + * without throwing any exceptions. + */ + @Test + void testValidExecutionBehavior_PerPlan() { + // Create a valid ExecutionBehavior with VARIABLE_EVALUATION_MODE_PER_PLAN + Plan.ExecutionBehavior executionBehavior = + ImmutableExecutionBehavior.builder() + .variableEvaluationMode( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .build(); + + // Create a Plan with the valid ExecutionBehavior + assertDoesNotThrow( + () -> { + ImmutablePlan.builder().executionBehavior(executionBehavior).build(); + }, + "Plan creation should succeed with valid ExecutionBehavior"); + } + + /** + * Test case 1b: Valid execution behavior with VARIABLE_EVALUATION_MODE_PER_RECORD. + * + *

This test verifies that a Plan with ExecutionBehavior set to + * VARIABLE_EVALUATION_MODE_PER_RECORD is also valid. + */ + @Test + void testValidExecutionBehavior_PerRecord() { + // Create a valid ExecutionBehavior with VARIABLE_EVALUATION_MODE_PER_RECORD + Plan.ExecutionBehavior executionBehavior = + ImmutableExecutionBehavior.builder() + .variableEvaluationMode( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD) + .build(); + + // Create a Plan with the valid ExecutionBehavior + assertDoesNotThrow( + () -> { + ImmutablePlan.builder().executionBehavior(executionBehavior).build(); + }, + "Plan creation should succeed with valid ExecutionBehavior"); + } + + /** + * Test case 2: Missing execution behavior (empty Optional). + * + *

This test verifies that attempting to create a Plan without setting the ExecutionBehavior + * field throws an IllegalArgumentException with an appropriate error message indicating that + * ExecutionBehavior is required. + */ + @Test + void testMissingExecutionBehavior() { + // Attempt to create a Plan without ExecutionBehavior + IllegalArgumentException exception = + assertThrows( + IllegalArgumentException.class, + () -> { + ImmutablePlan.builder().build(); + }, + "Plan creation should fail when ExecutionBehavior is not set"); + + // Verify the error message + assertEquals( + "ExecutionBehavior is required but was not set", + exception.getMessage(), + "Error message should indicate ExecutionBehavior is required"); + } + + /** + * Test case 3: Execution behavior with unspecified variable evaluation mode. + * + *

This test verifies that attempting to create a Plan with an ExecutionBehavior that has its + * VariableEvaluationMode set to VARIABLE_EVALUATION_MODE_UNSPECIFIED throws an + * IllegalArgumentException with an appropriate error message. + */ + @Test + void testUnspecifiedVariableEvaluationMode() { + // Create an ExecutionBehavior with VARIABLE_EVALUATION_MODE_UNSPECIFIED + Plan.ExecutionBehavior executionBehavior = + ImmutableExecutionBehavior.builder() + .variableEvaluationMode( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_UNSPECIFIED) + .build(); + + // Attempt to create a Plan with the invalid ExecutionBehavior + IllegalArgumentException exception = + assertThrows( + IllegalArgumentException.class, + () -> { + ImmutablePlan.builder().executionBehavior(executionBehavior).build(); + }, + "Plan creation should fail when VariableEvaluationMode is UNSPECIFIED"); + + // Verify the error message contains the expected information + String expectedMessage = + "ExecutionBehavior requires a specified VariableEvaluationMode, but got: " + + "VARIABLE_EVALUATION_MODE_UNSPECIFIED"; + assertEquals( + expectedMessage, + exception.getMessage(), + "Error message should indicate VariableEvaluationMode cannot be UNSPECIFIED"); + } +} diff --git a/core/src/test/resources/plan-roundtrip/complex-expected-plan.json b/core/src/test/resources/plan-roundtrip/complex-expected-plan.json index 9f4c47a18..99125c4f9 100644 --- a/core/src/test/resources/plan-roundtrip/complex-expected-plan.json +++ b/core/src/test/resources/plan-roundtrip/complex-expected-plan.json @@ -148,6 +148,9 @@ } ], "version": { - "minorNumber": 75 + "minorNumber": 87 + }, + "executionBehavior": { + "variableEvalMode": "VARIABLE_EVALUATION_MODE_PER_PLAN" } } diff --git a/core/src/test/resources/plan-roundtrip/complex-input-plan.json b/core/src/test/resources/plan-roundtrip/complex-input-plan.json index 17a1984bd..682c296cc 100644 --- a/core/src/test/resources/plan-roundtrip/complex-input-plan.json +++ b/core/src/test/resources/plan-roundtrip/complex-input-plan.json @@ -133,6 +133,9 @@ } ], "version": { - "minorNumber": 75 + "minorNumber": 87 + }, + "executionBehavior": { + "variableEvalMode": "VARIABLE_EVALUATION_MODE_PER_PLAN" } } diff --git a/core/src/test/resources/plan-roundtrip/simple-expected-plan.json b/core/src/test/resources/plan-roundtrip/simple-expected-plan.json index 5ae8daa3f..2edc69c1d 100644 --- a/core/src/test/resources/plan-roundtrip/simple-expected-plan.json +++ b/core/src/test/resources/plan-roundtrip/simple-expected-plan.json @@ -120,6 +120,9 @@ } ], "version": { - "minorNumber": 75 + "minorNumber": 87 + }, + "executionBehavior": { + "variableEvalMode": "VARIABLE_EVALUATION_MODE_PER_PLAN" } } diff --git a/core/src/test/resources/plan-roundtrip/simple-input-plan.json b/core/src/test/resources/plan-roundtrip/simple-input-plan.json index b5f5af06e..845b68dab 100644 --- a/core/src/test/resources/plan-roundtrip/simple-input-plan.json +++ b/core/src/test/resources/plan-roundtrip/simple-input-plan.json @@ -105,6 +105,9 @@ } ], "version": { - "minorNumber": 75 + "minorNumber": 87 + }, + "executionBehavior": { + "variableEvalMode": "VARIABLE_EVALUATION_MODE_PER_PLAN" } } diff --git a/core/src/test/resources/plan-roundtrip/zero-anchor-expected-plan.json b/core/src/test/resources/plan-roundtrip/zero-anchor-expected-plan.json index ff6b8ab0e..b60e6dd7b 100644 --- a/core/src/test/resources/plan-roundtrip/zero-anchor-expected-plan.json +++ b/core/src/test/resources/plan-roundtrip/zero-anchor-expected-plan.json @@ -92,6 +92,9 @@ } ], "version": { - "minorNumber": 75 + "minorNumber": 87 + }, + "executionBehavior": { + "variableEvalMode": "VARIABLE_EVALUATION_MODE_PER_PLAN" } } diff --git a/core/src/test/resources/plan-roundtrip/zero-anchor-input-plan.json b/core/src/test/resources/plan-roundtrip/zero-anchor-input-plan.json index 202814110..d8300a067 100644 --- a/core/src/test/resources/plan-roundtrip/zero-anchor-input-plan.json +++ b/core/src/test/resources/plan-roundtrip/zero-anchor-input-plan.json @@ -79,6 +79,9 @@ } ], "version": { - "minorNumber": 75 + "minorNumber": 87 + }, + "executionBehavior": { + "variableEvalMode": "VARIABLE_EVALUATION_MODE_PER_PLAN" } } From cad2925125a21ba33a7e96a95cc7509066d8ebc0 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Mon, 30 Mar 2026 11:39:58 +0200 Subject: [PATCH 02/19] feat(isthmus): add plan execution behavior Signed-off-by: Niels Pardon --- .../substrait/isthmus/ConverterProvider.java | 42 +++++++++++++++++++ .../io/substrait/isthmus/SqlToSubstrait.java | 2 + ...icDynamicFunctionMappingRoundtripTest.java | 8 +--- .../test/resources/lambdas/basic-lambda.json | 6 ++- .../resources/lambdas/lambda-field-ref.json | 6 +++ .../lambdas/lambda-with-function.json | 6 +++ 6 files changed, 62 insertions(+), 8 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java index b05d864bf..1f7c60222 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -12,6 +12,8 @@ import io.substrait.isthmus.expression.SqlArrayValueConstructorCallConverter; import io.substrait.isthmus.expression.SqlMapValueConstructorCallConverter; import io.substrait.isthmus.expression.WindowFunctionConverter; +import io.substrait.plan.ImmutableExecutionBehavior; +import io.substrait.plan.Plan; import io.substrait.relation.Rel; import java.util.ArrayList; import java.util.List; @@ -64,6 +66,8 @@ public class ConverterProvider { /** Converter for Substrait types to Calcite types and vice versa. */ protected TypeConverter typeConverter; + protected final Plan.ExecutionBehavior executionBehavior; + /** * Creates a ConverterProvider with default extension collection and type factory. Uses {@link * DefaultExtensionCatalog#DEFAULT_COLLECTION} and {@link SubstraitTypeSystem#TYPE_FACTORY}. @@ -115,12 +119,36 @@ public ConverterProvider( AggregateFunctionConverter afc, WindowFunctionConverter wfc, TypeConverter tc) { + this(typeFactory, extensions, sfc, afc, wfc, tc, createDefaultExecutionBehavior()); + } + + public ConverterProvider( + RelDataTypeFactory typeFactory, + SimpleExtension.ExtensionCollection extensions, + ScalarFunctionConverter sfc, + AggregateFunctionConverter afc, + WindowFunctionConverter wfc, + TypeConverter tc, + Plan.ExecutionBehavior executionBehavior) { this.typeFactory = typeFactory; this.extensions = extensions; this.scalarFunctionConverter = sfc; this.aggregateFunctionConverter = afc; this.windowFunctionConverter = wfc; this.typeConverter = tc; + this.executionBehavior = executionBehavior; + } + + /** + * Creates the default execution behavior with PER_PLAN variable evaluation mode. + * + * @return the default execution behavior + */ + private static Plan.ExecutionBehavior createDefaultExecutionBehavior() { + return ImmutableExecutionBehavior.builder() + .variableEvaluationMode( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .build(); } // SQL to Calcite Processing @@ -351,4 +379,18 @@ public WindowFunctionConverter getWindowFunctionConverter() { public TypeConverter getTypeConverter() { return typeConverter; } + + /** + * Returns the execution behavior for plans created by this converter. + * + *

The default execution behavior uses {@link + * Plan.ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_PER_PLAN}, which + * evaluates variables once per plan execution. This can be customized by providing a different + * execution behavior through the constructor. + * + * @return the execution behavior to use when creating plans + */ + public Plan.ExecutionBehavior getExecutionBehavior() { + return executionBehavior; + } } diff --git a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java index 0fb7cf1c3..765dd8dfd 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SqlToSubstrait.java @@ -47,6 +47,7 @@ public Plan convert(final String sqlStatements, final Prepare.CatalogReader cata throws SqlParseException { Builder builder = io.substrait.plan.Plan.builder(); builder.version(Version.builder().from(Version.DEFAULT_VERSION).producer("isthmus").build()); + builder.executionBehavior(converterProvider.getExecutionBehavior()); // TODO: consider case in which one sql passes conversion while others don't SubstraitSqlToCalcite.convertQueries(sqlStatements, catalogReader, operatorTable).stream() @@ -73,6 +74,7 @@ public Plan convert( throws SqlParseException { Builder builder = io.substrait.plan.Plan.builder(); builder.version(Version.builder().from(Version.DEFAULT_VERSION).producer("isthmus").build()); + builder.executionBehavior(converterProvider.getExecutionBehavior()); final SqlParser.Config sqlParserConfig = sqlDialect.configureParser( diff --git a/isthmus/src/test/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingRoundtripTest.java b/isthmus/src/test/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingRoundtripTest.java index 9538421e5..03f0c8f0f 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingRoundtripTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/AutomaticDynamicFunctionMappingRoundtripTest.java @@ -63,13 +63,9 @@ void testMultipleVariantsOfUnmappedFunctionRoundtrip() { sb.remap(2, 3), // The inputs are indices 0, 1. The two scalarFn outputs are 2, 3. table); - // Build plan with output field names + // Build plan with output field names and default execution behavior Plan plan = - Plan.builder() - .roots( - List.of( - Plan.Root.builder().input(project).names(List.of("ts_str", "dt_str")).build())) - .build(); + sb.plan(Plan.Root.builder().input(project).names(List.of("ts_str", "dt_str")).build()); // Use PlanTestBase helper method for comprehensive roundtrip testing assertFullRoundTrip(plan.getRoots().get(0)); diff --git a/isthmus/src/test/resources/lambdas/basic-lambda.json b/isthmus/src/test/resources/lambdas/basic-lambda.json index 0a542c74e..85b70cc00 100644 --- a/isthmus/src/test/resources/lambdas/basic-lambda.json +++ b/isthmus/src/test/resources/lambdas/basic-lambda.json @@ -1,7 +1,9 @@ { "version": { - "majorNumber": 0, - "minorNumber": 79 + "minorNumber": 87 + }, + "executionBehavior": { + "variableEvalMode": "VARIABLE_EVALUATION_MODE_PER_PLAN" }, "extensionUrns": [ { diff --git a/isthmus/src/test/resources/lambdas/lambda-field-ref.json b/isthmus/src/test/resources/lambdas/lambda-field-ref.json index a16657663..db4fd21d7 100644 --- a/isthmus/src/test/resources/lambdas/lambda-field-ref.json +++ b/isthmus/src/test/resources/lambdas/lambda-field-ref.json @@ -1,4 +1,10 @@ { + "version": { + "minorNumber": 87 + }, + "executionBehavior": { + "variableEvalMode": "VARIABLE_EVALUATION_MODE_PER_PLAN" + }, "extensionUrns": [ { "extensionUrnAnchor": 1, diff --git a/isthmus/src/test/resources/lambdas/lambda-with-function.json b/isthmus/src/test/resources/lambdas/lambda-with-function.json index f11451160..6252a2f63 100644 --- a/isthmus/src/test/resources/lambdas/lambda-with-function.json +++ b/isthmus/src/test/resources/lambdas/lambda-with-function.json @@ -1,4 +1,10 @@ { + "version": { + "minorNumber": 87 + }, + "executionBehavior": { + "variableEvalMode": "VARIABLE_EVALUATION_MODE_PER_PLAN" + }, "extensionUrns": [ { "extensionUrnAnchor": 1, From e30bdab3cbc7e9ab0b4db106f58220123c88ad72 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Mon, 30 Mar 2026 11:57:13 +0200 Subject: [PATCH 03/19] feat(spark): add plan execution behavior Signed-off-by: Niels Pardon --- .../scala/io/substrait/spark/logical/ToSubstraitRel.scala | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala b/spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala index d55e4c395..2d4d9fb88 100644 --- a/spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala +++ b/spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala @@ -47,7 +47,8 @@ import io.substrait.expression.{Expression => SExpression, ExpressionCreator} import io.substrait.expression.Expression.StructLiteral import io.substrait.extension.ExtensionCollector import io.substrait.hint.Hint -import io.substrait.plan.Plan +import io.substrait.plan.{ImmutableExecutionBehavior, Plan} +import io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode import io.substrait.relation.AbstractDdlRel.{DdlObject, DdlOp} import io.substrait.relation.AbstractWriteRel.{CreateMode, OutputMode, WriteOp} import io.substrait.relation.RelProtoConverter @@ -707,6 +708,10 @@ class ToSubstraitRel extends AbstractLogicalPlanVisitor with Logging { .from(Plan.Version.DEFAULT_VERSION) .producer("substrait-spark") .build()) + .executionBehavior( + ImmutableExecutionBehavior.builder() + .variableEvaluationMode(VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .build()) .roots( Collections.singletonList( Plan.Root From 56fdf51d6f4f716c24d9319dbcd1147395eef0d2 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Tue, 2 Jun 2026 15:00:22 +0200 Subject: [PATCH 04/19] docs: fix javadoc Signed-off-by: Niels Pardon --- .../java/io/substrait/isthmus/ConverterProvider.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java index 1f7c60222..19c6c4637 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -66,6 +66,7 @@ public class ConverterProvider { /** Converter for Substrait types to Calcite types and vice versa. */ protected TypeConverter typeConverter; + /** The execution behavior configuration for plans created by this converter. */ protected final Plan.ExecutionBehavior executionBehavior; /** @@ -122,6 +123,17 @@ public ConverterProvider( this(typeFactory, extensions, sfc, afc, wfc, tc, createDefaultExecutionBehavior()); } + /** + * Creates a ConverterProvider with full customization including execution behavior. + * + * @param typeFactory the Calcite type factory to use + * @param extensions the Substrait extension collection to use + * @param sfc the scalar function converter to use + * @param afc the aggregate function converter to use + * @param wfc the window function converter to use + * @param tc the type converter to use + * @param executionBehavior the execution behavior to use for plans + */ public ConverterProvider( RelDataTypeFactory typeFactory, SimpleExtension.ExtensionCollection extensions, From 32b893353e8fe7d4f82e47af03556fb910da8481 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Tue, 2 Jun 2026 15:01:10 +0200 Subject: [PATCH 05/19] test(spark): fix testcase Signed-off-by: Niels Pardon --- .../scala/io/substrait/spark/AggregateWithJoinSuite.scala | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spark/src/test/scala/io/substrait/spark/AggregateWithJoinSuite.scala b/spark/src/test/scala/io/substrait/spark/AggregateWithJoinSuite.scala index e65a4e769..a669eeedf 100644 --- a/spark/src/test/scala/io/substrait/spark/AggregateWithJoinSuite.scala +++ b/spark/src/test/scala/io/substrait/spark/AggregateWithJoinSuite.scala @@ -242,6 +242,11 @@ class AggregateWithJoinSuite extends SparkFunSuite with SharedSparkSession with val plan = Plan.builder() .addRoots(root) + .executionBehavior( + Plan.ExecutionBehavior.builder() + .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .build() + ) .build() val converter = new ToLogicalPlan(spark) From 5442086b4fbe62ace51c9c58c28b374d1cab470f Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Wed, 3 Jun 2026 16:53:11 +0200 Subject: [PATCH 06/19] feat: validate only when plan gets serialized to proto Signed-off-by: Niels Pardon --- core/src/main/java/io/substrait/plan/Plan.java | 14 +++++++++++--- .../java/io/substrait/plan/PlanProtoConverter.java | 3 +++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/io/substrait/plan/Plan.java b/core/src/main/java/io/substrait/plan/Plan.java index a3f897632..cc11a516b 100644 --- a/core/src/main/java/io/substrait/plan/Plan.java +++ b/core/src/main/java/io/substrait/plan/Plan.java @@ -35,14 +35,22 @@ public Version getVersion() { * ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_UNSPECIFIED}) * * + *

The validation is currently triggered when the Plan gets serialized into protobuf in {@link + * PlanProtoConverter#toProto(Plan)}. + * + *

TODO: After the grace period of introducing this breaking change is over, the validation + * should also be triggered when the Plan gets deserialized from protobuf by adding the {@link + * Value.Check} annotation to this method and removing the call from {@link + * PlanProtoConverter#toProto(Plan)}. + * * @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() { + protected void validate() { if (!getExecutionBehavior().isPresent()) { - throw new IllegalArgumentException("ExecutionBehavior is required but was not set"); + throw new IllegalArgumentException( + "ExecutionBehavior is required since Substrait v0.87.0 but was not set"); } ExecutionBehavior behavior = getExecutionBehavior().get(); if (behavior.getVariableEvaluationMode() diff --git a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java index d463999a8..1345495b9 100644 --- a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java +++ b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java @@ -84,6 +84,9 @@ public PlanProtoConverter( * @throws IllegalArgumentException if the plan contains invalid data */ public Plan toProto(final io.substrait.plan.Plan plan) { + // validate Plan is valid with current specification + plan.validate(); + final List planRels = new ArrayList<>(); final ExtensionCollector functionCollector = new ExtensionCollector(extensionCollection); for (final io.substrait.plan.Plan.Root root : plan.getRoots()) { From 30e38724f48758855bc7e4dac6df75f36734a662 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Wed, 3 Jun 2026 17:06:40 +0200 Subject: [PATCH 07/19] test: update tests Signed-off-by: Niels Pardon --- .../io/substrait/plan/PlanConverterTest.java | 41 +++++++++++-------- .../io/substrait/plan/PlanValidationTest.java | 36 ++++++++-------- 2 files changed, 39 insertions(+), 38 deletions(-) diff --git a/core/src/test/java/io/substrait/plan/PlanConverterTest.java b/core/src/test/java/io/substrait/plan/PlanConverterTest.java index 0c2a7f9ba..228498057 100644 --- a/core/src/test/java/io/substrait/plan/PlanConverterTest.java +++ b/core/src/test/java/io/substrait/plan/PlanConverterTest.java @@ -475,30 +475,33 @@ void testFromProtoWithExecutionBehaviorPerRecord() { /** * Conversion from protobuf Plan without ExecutionBehavior. * - *

Verifies that a protobuf Plan without ExecutionBehavior results in a POJO Plan that fails - * validation (since ExecutionBehavior is required). + *

Verifies that converting a protobuf Plan without ExecutionBehavior to POJO succeeds, but + * serializing that POJO via + * [`PlanProtoConverter.toProto()`](core/src/main/java/io/substrait/plan/PlanProtoConverter.java:86) + * fails validation because ExecutionBehavior is required. */ @Test void testFromProtoWithoutExecutionBehaviorFailsValidation() { - // Create a protobuf Plan without ExecutionBehavior io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); - // Attempt to convert to POJO - should fail validation + Plan plan = fromProtoConverter.from(protoPlan); + assertThrows( IllegalArgumentException.class, - () -> fromProtoConverter.from(protoPlan), - "Conversion should fail when ExecutionBehavior is not set"); + () -> toProtoConverter.toProto(plan), + "Serialization should fail when ExecutionBehavior is not set"); } /** * Test case 6: Conversion from protobuf Plan with UNSPECIFIED mode fails validation. * - *

Verifies that a protobuf Plan with VARIABLE_EVALUATION_MODE_UNSPECIFIED results in a - * validation failure when converted to POJO. + *

Verifies that converting a protobuf Plan with VARIABLE_EVALUATION_MODE_UNSPECIFIED to POJO + * succeeds, but serializing that POJO via + * [`PlanProtoConverter.toProto()`](core/src/main/java/io/substrait/plan/PlanProtoConverter.java:86) + * fails validation. */ @Test void testFromProtoWithUnspecifiedModeFailsValidation() { - // Create a protobuf Plan with UNSPECIFIED mode io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder() .setExecutionBehavior( @@ -509,12 +512,13 @@ void testFromProtoWithUnspecifiedModeFailsValidation() { .build()) .build(); - // Attempt to convert to POJO - should fail validation + Plan plan = fromProtoConverter.from(protoPlan); + IllegalArgumentException exception = assertThrows( IllegalArgumentException.class, - () -> fromProtoConverter.from(protoPlan), - "Conversion should fail when VariableEvaluationMode is UNSPECIFIED"); + () -> toProtoConverter.toProto(plan), + "Serialization should fail when VariableEvaluationMode is UNSPECIFIED"); assertTrue( exception.getMessage().contains("VariableEvaluationMode"), @@ -637,21 +641,22 @@ void testRoundTripEmptyPlanWithExecutionBehavior() { * Verify that protobuf without execution behavior field is handled. * *

Tests the edge case where a protobuf Plan doesn't have the execution behavior field set at - * all. + * all. Conversion to POJO should succeed, but serialization via + * [`PlanProtoConverter.toProto()`](core/src/main/java/io/substrait/plan/PlanProtoConverter.java:86) + * should fail. */ @Test void testFromProtoMissingExecutionBehaviorField() { - // Create protobuf Plan without setting execution behavior io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); - // Verify hasExecutionBehavior returns false assertFalse( protoPlan.hasExecutionBehavior(), "Protobuf Plan should not have execution behavior field"); - // Conversion should fail validation + Plan plan = fromProtoConverter.from(protoPlan); + assertThrows( IllegalArgumentException.class, - () -> fromProtoConverter.from(protoPlan), - "Conversion should fail when execution behavior is missing"); + () -> toProtoConverter.toProto(plan), + "Serialization should fail when execution behavior is missing"); } } diff --git a/core/src/test/java/io/substrait/plan/PlanValidationTest.java b/core/src/test/java/io/substrait/plan/PlanValidationTest.java index 906074ed6..b9ac978fa 100644 --- a/core/src/test/java/io/substrait/plan/PlanValidationTest.java +++ b/core/src/test/java/io/substrait/plan/PlanValidationTest.java @@ -61,24 +61,22 @@ void testValidExecutionBehavior_PerRecord() { /** * Test case 2: Missing execution behavior (empty Optional). * - *

This test verifies that attempting to create a Plan without setting the ExecutionBehavior - * field throws an IllegalArgumentException with an appropriate error message indicating that - * ExecutionBehavior is required. + *

This test verifies that a Plan can be built without ExecutionBehavior, but serialization via + * [`PlanProtoConverter.toProto()`](core/src/main/java/io/substrait/plan/PlanProtoConverter.java:86) + * fails with an IllegalArgumentException indicating that ExecutionBehavior is required. */ @Test void testMissingExecutionBehavior() { - // Attempt to create a Plan without ExecutionBehavior + Plan plan = ImmutablePlan.builder().build(); + IllegalArgumentException exception = assertThrows( IllegalArgumentException.class, - () -> { - ImmutablePlan.builder().build(); - }, - "Plan creation should fail when ExecutionBehavior is not set"); + () -> new PlanProtoConverter().toProto(plan), + "Plan serialization should fail when ExecutionBehavior is not set"); - // Verify the error message assertEquals( - "ExecutionBehavior is required but was not set", + "ExecutionBehavior is required since Substrait v0.87.0 but was not set", exception.getMessage(), "Error message should indicate ExecutionBehavior is required"); } @@ -86,29 +84,27 @@ void testMissingExecutionBehavior() { /** * Test case 3: Execution behavior with unspecified variable evaluation mode. * - *

This test verifies that attempting to create a Plan with an ExecutionBehavior that has its - * VariableEvaluationMode set to VARIABLE_EVALUATION_MODE_UNSPECIFIED throws an - * IllegalArgumentException with an appropriate error message. + *

This test verifies that a Plan can be built with an ExecutionBehavior whose + * VariableEvaluationMode is VARIABLE_EVALUATION_MODE_UNSPECIFIED, but serialization via + * [`PlanProtoConverter.toProto()`](core/src/main/java/io/substrait/plan/PlanProtoConverter.java:86) + * fails with an IllegalArgumentException. */ @Test void testUnspecifiedVariableEvaluationMode() { - // Create an ExecutionBehavior with VARIABLE_EVALUATION_MODE_UNSPECIFIED Plan.ExecutionBehavior executionBehavior = ImmutableExecutionBehavior.builder() .variableEvaluationMode( Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_UNSPECIFIED) .build(); - // Attempt to create a Plan with the invalid ExecutionBehavior + Plan plan = ImmutablePlan.builder().executionBehavior(executionBehavior).build(); + IllegalArgumentException exception = assertThrows( IllegalArgumentException.class, - () -> { - ImmutablePlan.builder().executionBehavior(executionBehavior).build(); - }, - "Plan creation should fail when VariableEvaluationMode is UNSPECIFIED"); + () -> new PlanProtoConverter().toProto(plan), + "Plan serialization should fail when VariableEvaluationMode is UNSPECIFIED"); - // Verify the error message contains the expected information String expectedMessage = "ExecutionBehavior requires a specified VariableEvaluationMode, but got: " + "VARIABLE_EVALUATION_MODE_UNSPECIFIED"; From b9831b69d3b5c6bd8fc25949980c761bcd288b74 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Fri, 5 Jun 2026 09:06:11 +0200 Subject: [PATCH 08/19] chore: revert prior commits - "test: update tests" - "feat: validate only when plan gets serialized to proto" --- .../src/main/java/io/substrait/plan/Plan.java | 14 ++----- .../io/substrait/plan/PlanProtoConverter.java | 3 -- .../io/substrait/plan/PlanConverterTest.java | 41 ++++++++----------- .../io/substrait/plan/PlanValidationTest.java | 36 ++++++++-------- 4 files changed, 41 insertions(+), 53 deletions(-) diff --git a/core/src/main/java/io/substrait/plan/Plan.java b/core/src/main/java/io/substrait/plan/Plan.java index cc11a516b..a3f897632 100644 --- a/core/src/main/java/io/substrait/plan/Plan.java +++ b/core/src/main/java/io/substrait/plan/Plan.java @@ -35,22 +35,14 @@ public Version getVersion() { * ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_UNSPECIFIED}) * * - *

The validation is currently triggered when the Plan gets serialized into protobuf in {@link - * PlanProtoConverter#toProto(Plan)}. - * - *

TODO: After the grace period of introducing this breaking change is over, the validation - * should also be triggered when the Plan gets deserialized from protobuf by adding the {@link - * Value.Check} annotation to this method and removing the call from {@link - * PlanProtoConverter#toProto(Plan)}. - * * @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} */ - protected void validate() { + @Value.Check + protected void check() { if (!getExecutionBehavior().isPresent()) { - throw new IllegalArgumentException( - "ExecutionBehavior is required since Substrait v0.87.0 but was not set"); + throw new IllegalArgumentException("ExecutionBehavior is required but was not set"); } ExecutionBehavior behavior = getExecutionBehavior().get(); if (behavior.getVariableEvaluationMode() diff --git a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java index 1345495b9..d463999a8 100644 --- a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java +++ b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java @@ -84,9 +84,6 @@ public PlanProtoConverter( * @throws IllegalArgumentException if the plan contains invalid data */ public Plan toProto(final io.substrait.plan.Plan plan) { - // validate Plan is valid with current specification - plan.validate(); - final List planRels = new ArrayList<>(); final ExtensionCollector functionCollector = new ExtensionCollector(extensionCollection); for (final io.substrait.plan.Plan.Root root : plan.getRoots()) { diff --git a/core/src/test/java/io/substrait/plan/PlanConverterTest.java b/core/src/test/java/io/substrait/plan/PlanConverterTest.java index 228498057..0c2a7f9ba 100644 --- a/core/src/test/java/io/substrait/plan/PlanConverterTest.java +++ b/core/src/test/java/io/substrait/plan/PlanConverterTest.java @@ -475,33 +475,30 @@ void testFromProtoWithExecutionBehaviorPerRecord() { /** * Conversion from protobuf Plan without ExecutionBehavior. * - *

Verifies that converting a protobuf Plan without ExecutionBehavior to POJO succeeds, but - * serializing that POJO via - * [`PlanProtoConverter.toProto()`](core/src/main/java/io/substrait/plan/PlanProtoConverter.java:86) - * fails validation because ExecutionBehavior is required. + *

Verifies that a protobuf Plan without ExecutionBehavior results in a POJO Plan that fails + * validation (since ExecutionBehavior is required). */ @Test void testFromProtoWithoutExecutionBehaviorFailsValidation() { + // Create a protobuf Plan without ExecutionBehavior io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); - Plan plan = fromProtoConverter.from(protoPlan); - + // Attempt to convert to POJO - should fail validation assertThrows( IllegalArgumentException.class, - () -> toProtoConverter.toProto(plan), - "Serialization should fail when ExecutionBehavior is not set"); + () -> fromProtoConverter.from(protoPlan), + "Conversion should fail when ExecutionBehavior is not set"); } /** * Test case 6: Conversion from protobuf Plan with UNSPECIFIED mode fails validation. * - *

Verifies that converting a protobuf Plan with VARIABLE_EVALUATION_MODE_UNSPECIFIED to POJO - * succeeds, but serializing that POJO via - * [`PlanProtoConverter.toProto()`](core/src/main/java/io/substrait/plan/PlanProtoConverter.java:86) - * fails validation. + *

Verifies that a protobuf Plan with VARIABLE_EVALUATION_MODE_UNSPECIFIED results in a + * validation failure when converted to POJO. */ @Test void testFromProtoWithUnspecifiedModeFailsValidation() { + // Create a protobuf Plan with UNSPECIFIED mode io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder() .setExecutionBehavior( @@ -512,13 +509,12 @@ void testFromProtoWithUnspecifiedModeFailsValidation() { .build()) .build(); - Plan plan = fromProtoConverter.from(protoPlan); - + // Attempt to convert to POJO - should fail validation IllegalArgumentException exception = assertThrows( IllegalArgumentException.class, - () -> toProtoConverter.toProto(plan), - "Serialization should fail when VariableEvaluationMode is UNSPECIFIED"); + () -> fromProtoConverter.from(protoPlan), + "Conversion should fail when VariableEvaluationMode is UNSPECIFIED"); assertTrue( exception.getMessage().contains("VariableEvaluationMode"), @@ -641,22 +637,21 @@ void testRoundTripEmptyPlanWithExecutionBehavior() { * Verify that protobuf without execution behavior field is handled. * *

Tests the edge case where a protobuf Plan doesn't have the execution behavior field set at - * all. Conversion to POJO should succeed, but serialization via - * [`PlanProtoConverter.toProto()`](core/src/main/java/io/substrait/plan/PlanProtoConverter.java:86) - * should fail. + * all. */ @Test void testFromProtoMissingExecutionBehaviorField() { + // Create protobuf Plan without setting execution behavior io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); + // Verify hasExecutionBehavior returns false assertFalse( protoPlan.hasExecutionBehavior(), "Protobuf Plan should not have execution behavior field"); - Plan plan = fromProtoConverter.from(protoPlan); - + // Conversion should fail validation assertThrows( IllegalArgumentException.class, - () -> toProtoConverter.toProto(plan), - "Serialization should fail when execution behavior is missing"); + () -> fromProtoConverter.from(protoPlan), + "Conversion should fail when execution behavior is missing"); } } diff --git a/core/src/test/java/io/substrait/plan/PlanValidationTest.java b/core/src/test/java/io/substrait/plan/PlanValidationTest.java index b9ac978fa..906074ed6 100644 --- a/core/src/test/java/io/substrait/plan/PlanValidationTest.java +++ b/core/src/test/java/io/substrait/plan/PlanValidationTest.java @@ -61,22 +61,24 @@ void testValidExecutionBehavior_PerRecord() { /** * Test case 2: Missing execution behavior (empty Optional). * - *

This test verifies that a Plan can be built without ExecutionBehavior, but serialization via - * [`PlanProtoConverter.toProto()`](core/src/main/java/io/substrait/plan/PlanProtoConverter.java:86) - * fails with an IllegalArgumentException indicating that ExecutionBehavior is required. + *

This test verifies that attempting to create a Plan without setting the ExecutionBehavior + * field throws an IllegalArgumentException with an appropriate error message indicating that + * ExecutionBehavior is required. */ @Test void testMissingExecutionBehavior() { - Plan plan = ImmutablePlan.builder().build(); - + // Attempt to create a Plan without ExecutionBehavior IllegalArgumentException exception = assertThrows( IllegalArgumentException.class, - () -> new PlanProtoConverter().toProto(plan), - "Plan serialization should fail when ExecutionBehavior is not set"); + () -> { + ImmutablePlan.builder().build(); + }, + "Plan creation should fail when ExecutionBehavior is not set"); + // Verify the error message assertEquals( - "ExecutionBehavior is required since Substrait v0.87.0 but was not set", + "ExecutionBehavior is required but was not set", exception.getMessage(), "Error message should indicate ExecutionBehavior is required"); } @@ -84,27 +86,29 @@ void testMissingExecutionBehavior() { /** * Test case 3: Execution behavior with unspecified variable evaluation mode. * - *

This test verifies that a Plan can be built with an ExecutionBehavior whose - * VariableEvaluationMode is VARIABLE_EVALUATION_MODE_UNSPECIFIED, but serialization via - * [`PlanProtoConverter.toProto()`](core/src/main/java/io/substrait/plan/PlanProtoConverter.java:86) - * fails with an IllegalArgumentException. + *

This test verifies that attempting to create a Plan with an ExecutionBehavior that has its + * VariableEvaluationMode set to VARIABLE_EVALUATION_MODE_UNSPECIFIED throws an + * IllegalArgumentException with an appropriate error message. */ @Test void testUnspecifiedVariableEvaluationMode() { + // Create an ExecutionBehavior with VARIABLE_EVALUATION_MODE_UNSPECIFIED Plan.ExecutionBehavior executionBehavior = ImmutableExecutionBehavior.builder() .variableEvaluationMode( Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_UNSPECIFIED) .build(); - Plan plan = ImmutablePlan.builder().executionBehavior(executionBehavior).build(); - + // Attempt to create a Plan with the invalid ExecutionBehavior IllegalArgumentException exception = assertThrows( IllegalArgumentException.class, - () -> new PlanProtoConverter().toProto(plan), - "Plan serialization should fail when VariableEvaluationMode is UNSPECIFIED"); + () -> { + ImmutablePlan.builder().executionBehavior(executionBehavior).build(); + }, + "Plan creation should fail when VariableEvaluationMode is UNSPECIFIED"); + // Verify the error message contains the expected information String expectedMessage = "ExecutionBehavior requires a specified VariableEvaluationMode, but got: " + "VARIABLE_EVALUATION_MODE_UNSPECIFIED"; From 7ed2da45d24af03822454e5263534915e23a9c54 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Fri, 5 Jun 2026 09:07:58 +0200 Subject: [PATCH 09/19] feat: set default for older plan versions Signed-off-by: Niels Pardon --- .../io/substrait/plan/ProtoPlanConverter.java | 31 ++++- .../io/substrait/plan/PlanConverterTest.java | 14 ++- .../io/substrait/plan/PlanValidationTest.java | 114 ++++++++++++++++++ 3 files changed, 154 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java index cb719b6c9..53b859d27 100644 --- a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java +++ b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java @@ -126,14 +126,43 @@ public Plan from(io.substrait.proto.Plan plan) { : null)) .version(versionBuilder.build()); - // Set execution behavior if present + // Set execution behavior if present. For plans produced before Substrait 0.87.0 the + // execution behavior did not exist, so default it to per-plan variable evaluation. if (plan.hasExecutionBehavior()) { planBuilder.executionBehavior(fromProtoExecutionBehavior(plan.getExecutionBehavior())); + } else if (isOlderThan(plan.getVersion(), 0, 87, 0)) { + planBuilder.executionBehavior( + io.substrait.plan.Plan.ExecutionBehavior.builder() + .variableEvaluationMode( + io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode + .VARIABLE_EVALUATION_MODE_PER_PLAN) + .build()); } return planBuilder.build(); } + /** + * Determines whether the given protobuf {@link io.substrait.proto.Version} is older than the + * provided major/minor/patch version. + * + * @param version the protobuf Version to check + * @param major the major version to compare against + * @param minor the minor version to compare against + * @param patch the patch version to compare against + * @return {@code true} if {@code version} is strictly older than {@code major.minor.patch} + */ + private static boolean isOlderThan( + final io.substrait.proto.Version version, final int major, final int minor, final int patch) { + if (version.getMajorNumber() != major) { + return version.getMajorNumber() < major; + } + if (version.getMinorNumber() != minor) { + return version.getMinorNumber() < minor; + } + return version.getPatchNumber() < patch; + } + /** * Converts a protobuf {@link io.substrait.proto.ExecutionBehavior} to its POJO representation. * diff --git a/core/src/test/java/io/substrait/plan/PlanConverterTest.java b/core/src/test/java/io/substrait/plan/PlanConverterTest.java index 0c2a7f9ba..be9bf6215 100644 --- a/core/src/test/java/io/substrait/plan/PlanConverterTest.java +++ b/core/src/test/java/io/substrait/plan/PlanConverterTest.java @@ -480,8 +480,11 @@ void testFromProtoWithExecutionBehaviorPerRecord() { */ @Test void testFromProtoWithoutExecutionBehaviorFailsValidation() { - // Create a protobuf Plan without ExecutionBehavior - io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); + // Create a protobuf Plan without ExecutionBehavior at a version that requires it (>= 0.87.0) + io.substrait.proto.Plan protoPlan = + io.substrait.proto.Plan.newBuilder() + .setVersion(io.substrait.proto.Version.newBuilder().setMinorNumber(87).build()) + .build(); // Attempt to convert to POJO - should fail validation assertThrows( @@ -641,8 +644,11 @@ void testRoundTripEmptyPlanWithExecutionBehavior() { */ @Test void testFromProtoMissingExecutionBehaviorField() { - // Create protobuf Plan without setting execution behavior - io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); + // Create protobuf Plan without setting execution behavior at a version that requires it + io.substrait.proto.Plan protoPlan = + io.substrait.proto.Plan.newBuilder() + .setVersion(io.substrait.proto.Version.newBuilder().setMinorNumber(87).build()) + .build(); // Verify hasExecutionBehavior returns false assertFalse( diff --git a/core/src/test/java/io/substrait/plan/PlanValidationTest.java b/core/src/test/java/io/substrait/plan/PlanValidationTest.java index 906074ed6..b34273b85 100644 --- a/core/src/test/java/io/substrait/plan/PlanValidationTest.java +++ b/core/src/test/java/io/substrait/plan/PlanValidationTest.java @@ -3,6 +3,7 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.Test; @@ -11,6 +12,16 @@ */ class PlanValidationTest { + private final ProtoPlanConverter fromProtoConverter = new ProtoPlanConverter(); + + private static io.substrait.proto.Version protoVersion(int major, int minor, int patch) { + return io.substrait.proto.Version.newBuilder() + .setMajorNumber(major) + .setMinorNumber(minor) + .setPatchNumber(patch) + .build(); + } + /** * Test case 1: Valid execution behavior with proper variable evaluation mode. * @@ -117,4 +128,107 @@ void testUnspecifiedVariableEvaluationMode() { exception.getMessage(), "Error message should indicate VariableEvaluationMode cannot be UNSPECIFIED"); } + + /** + * Test case 4: Missing execution behavior on a pre-0.87.0 plan defaults to PER_PLAN. + * + *

ExecutionBehavior did not exist before Substrait 0.87.0, so converting a protobuf Plan that + * explicitly declares an older version and omits execution behavior should default the variable + * evaluation mode to PER_PLAN rather than failing validation. + */ + @Test + void testMissingExecutionBehaviorOnOldVersionDefaultsToPerPlan() { + io.substrait.proto.Plan protoPlan = + io.substrait.proto.Plan.newBuilder().setVersion(protoVersion(0, 86, 0)).build(); + + Plan plan = assertDoesNotThrow(() -> fromProtoConverter.from(protoPlan)); + + assertTrue( + plan.getExecutionBehavior().isPresent(), + "Pre-0.87.0 plan should have a defaulted ExecutionBehavior"); + assertEquals( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, + plan.getExecutionBehavior().get().getVariableEvaluationMode(), + "Pre-0.87.0 plan should default to PER_PLAN"); + } + + /** + * Test case 5: Missing execution behavior at exactly 0.87.0 still fails validation. + * + *

ExecutionBehavior is required from Substrait 0.87.0 onward, so a plan at that version with + * no execution behavior must fail validation rather than being defaulted. + */ + @Test + void testMissingExecutionBehaviorAtCutoffVersionFails() { + io.substrait.proto.Plan protoPlan = + io.substrait.proto.Plan.newBuilder().setVersion(protoVersion(0, 87, 0)).build(); + + IllegalArgumentException exception = + assertThrows(IllegalArgumentException.class, () -> fromProtoConverter.from(protoPlan)); + assertEquals( + "ExecutionBehavior is required but was not set", + exception.getMessage(), + "Plans at or after 0.87.0 require ExecutionBehavior"); + } + + /** + * Test case 6: Missing execution behavior on a newer plan still fails validation. + * + *

Verifies the requirement also holds for versions beyond the 0.87.0 cutoff. + */ + @Test + void testMissingExecutionBehaviorOnNewerVersionFails() { + io.substrait.proto.Plan protoPlan = + io.substrait.proto.Plan.newBuilder().setVersion(protoVersion(1, 0, 0)).build(); + + assertThrows(IllegalArgumentException.class, () -> fromProtoConverter.from(protoPlan)); + } + + /** + * Test case 7: A version-less plan reads as the protobuf default 0.0.0. + * + *

A protobuf Plan that does not declare a version at all has the default version 0.0.0, which + * is older than 0.87.0, so a missing execution behavior is defaulted to PER_PLAN. + */ + @Test + void testMissingExecutionBehaviorWithoutVersionDefaultsToPerPlan() { + io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); + + Plan plan = assertDoesNotThrow(() -> fromProtoConverter.from(protoPlan)); + + assertTrue( + plan.getExecutionBehavior().isPresent(), + "Version-less plan (0.0.0) should have a defaulted ExecutionBehavior"); + assertEquals( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, + plan.getExecutionBehavior().get().getVariableEvaluationMode(), + "Version-less plan (0.0.0) should default to PER_PLAN"); + } + + /** + * Test case 8: An explicit execution behavior on an old plan is not overridden. + * + *

When a pre-0.87.0 plan does provide an execution behavior, the provided value must be used + * instead of the PER_PLAN default. + */ + @Test + void testExplicitExecutionBehaviorOnOldVersionIsPreserved() { + io.substrait.proto.Plan protoPlan = + io.substrait.proto.Plan.newBuilder() + .setVersion(protoVersion(0, 86, 0)) + .setExecutionBehavior( + io.substrait.proto.ExecutionBehavior.newBuilder() + .setVariableEvalMode( + io.substrait.proto.ExecutionBehavior.VariableEvaluationMode + .VARIABLE_EVALUATION_MODE_PER_RECORD) + .build()) + .build(); + + Plan plan = fromProtoConverter.from(protoPlan); + + assertEquals( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD, + plan.getExecutionBehavior().get().getVariableEvaluationMode(), + "Explicit execution behavior should not be overridden by the default"); + } } From 32bee922864c1faef64c558a6417a44a301601bf Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Fri, 5 Jun 2026 09:17:13 +0200 Subject: [PATCH 10/19] docs: apply suggestions from code review Co-authored-by: Ben Bellick <36523439+benbellick@users.noreply.github.com> --- .../java/io/substrait/plan/PlanProtoConverter.java | 12 ------------ .../java/io/substrait/plan/ProtoPlanConverter.java | 9 --------- 2 files changed, 21 deletions(-) diff --git a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java index d463999a8..d324b6d71 100644 --- a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java +++ b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java @@ -66,18 +66,6 @@ public PlanProtoConverter( /** * Converts a {@link io.substrait.plan.Plan} object to its protobuf representation. * - *

This method performs a complete conversion of the Plan object, including: - * - *

- * - *

The execution behavior is optional. If present, it will be converted using {@link - * #toProtoExecutionBehavior(io.substrait.plan.Plan.ExecutionBehavior)}. * * @param plan the Plan object to convert, must not be null * @return the protobuf Plan representation diff --git a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java index 53b859d27..10d9c7758 100644 --- a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java +++ b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java @@ -69,15 +69,6 @@ protected ProtoRelConverter getProtoRelConverter(final ExtensionLookup functionL /** * Converts a protobuf {@link io.substrait.proto.Plan} to a {@link Plan} POJO. * - *

This method performs a complete conversion from the protobuf representation, including: - * - *

* *

Note: While execution behavior is optional in the protobuf message, the Plan * validation will still fail if ExecutionBehavior is not set or if it contains From cda73684835d861c19f09fe3fb42901d4518b1a2 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Fri, 5 Jun 2026 09:20:59 +0200 Subject: [PATCH 11/19] docs: update javadocs Signed-off-by: Niels Pardon --- .../io/substrait/plan/PlanProtoConverter.java | 1 - .../io/substrait/plan/ProtoPlanConverter.java | 16 ++++++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java index d324b6d71..5bb61749d 100644 --- a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java +++ b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java @@ -66,7 +66,6 @@ public PlanProtoConverter( /** * Converts a {@link io.substrait.plan.Plan} object to its protobuf representation. * - * * @param plan the Plan object to convert, must not be null * @return the protobuf Plan representation * @throws IllegalArgumentException if the plan contains invalid data diff --git a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java index 10d9c7758..e9c72368b 100644 --- a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java +++ b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java @@ -69,11 +69,19 @@ protected ProtoRelConverter getProtoRelConverter(final ExtensionLookup functionL /** * Converts a protobuf {@link io.substrait.proto.Plan} to a {@link Plan} POJO. * + *

Note: 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: * - *

Note: While execution behavior is optional in the protobuf message, the Plan - * validation will still fail if ExecutionBehavior is not set or if it contains - * VARIABLE_EVALUATION_MODE_UNSPECIFIED. Ensure the protobuf Plan has a valid execution behavior - * before conversion. + *

* * @param plan the protobuf Plan to convert, must not be null * @return the converted Plan POJO From e9203b81e65f636a371b7ba6d2350953a31c729e Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Mon, 8 Jun 2026 10:21:52 +0200 Subject: [PATCH 12/19] fix: revert "feat: set default for older plan versions" This reverts commit 7ed2da45d24af03822454e5263534915e23a9c54. --- .../io/substrait/plan/ProtoPlanConverter.java | 31 +---- .../io/substrait/plan/PlanConverterTest.java | 14 +-- .../io/substrait/plan/PlanValidationTest.java | 114 ------------------ 3 files changed, 5 insertions(+), 154 deletions(-) diff --git a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java index e9c72368b..b23f9e21c 100644 --- a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java +++ b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java @@ -125,43 +125,14 @@ public Plan from(io.substrait.proto.Plan plan) { : null)) .version(versionBuilder.build()); - // Set execution behavior if present. For plans produced before Substrait 0.87.0 the - // execution behavior did not exist, so default it to per-plan variable evaluation. + // Set execution behavior if present if (plan.hasExecutionBehavior()) { planBuilder.executionBehavior(fromProtoExecutionBehavior(plan.getExecutionBehavior())); - } else if (isOlderThan(plan.getVersion(), 0, 87, 0)) { - planBuilder.executionBehavior( - io.substrait.plan.Plan.ExecutionBehavior.builder() - .variableEvaluationMode( - io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode - .VARIABLE_EVALUATION_MODE_PER_PLAN) - .build()); } return planBuilder.build(); } - /** - * Determines whether the given protobuf {@link io.substrait.proto.Version} is older than the - * provided major/minor/patch version. - * - * @param version the protobuf Version to check - * @param major the major version to compare against - * @param minor the minor version to compare against - * @param patch the patch version to compare against - * @return {@code true} if {@code version} is strictly older than {@code major.minor.patch} - */ - private static boolean isOlderThan( - final io.substrait.proto.Version version, final int major, final int minor, final int patch) { - if (version.getMajorNumber() != major) { - return version.getMajorNumber() < major; - } - if (version.getMinorNumber() != minor) { - return version.getMinorNumber() < minor; - } - return version.getPatchNumber() < patch; - } - /** * Converts a protobuf {@link io.substrait.proto.ExecutionBehavior} to its POJO representation. * diff --git a/core/src/test/java/io/substrait/plan/PlanConverterTest.java b/core/src/test/java/io/substrait/plan/PlanConverterTest.java index be9bf6215..0c2a7f9ba 100644 --- a/core/src/test/java/io/substrait/plan/PlanConverterTest.java +++ b/core/src/test/java/io/substrait/plan/PlanConverterTest.java @@ -480,11 +480,8 @@ void testFromProtoWithExecutionBehaviorPerRecord() { */ @Test void testFromProtoWithoutExecutionBehaviorFailsValidation() { - // Create a protobuf Plan without ExecutionBehavior at a version that requires it (>= 0.87.0) - io.substrait.proto.Plan protoPlan = - io.substrait.proto.Plan.newBuilder() - .setVersion(io.substrait.proto.Version.newBuilder().setMinorNumber(87).build()) - .build(); + // Create a protobuf Plan without ExecutionBehavior + io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); // Attempt to convert to POJO - should fail validation assertThrows( @@ -644,11 +641,8 @@ void testRoundTripEmptyPlanWithExecutionBehavior() { */ @Test void testFromProtoMissingExecutionBehaviorField() { - // Create protobuf Plan without setting execution behavior at a version that requires it - io.substrait.proto.Plan protoPlan = - io.substrait.proto.Plan.newBuilder() - .setVersion(io.substrait.proto.Version.newBuilder().setMinorNumber(87).build()) - .build(); + // Create protobuf Plan without setting execution behavior + io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); // Verify hasExecutionBehavior returns false assertFalse( diff --git a/core/src/test/java/io/substrait/plan/PlanValidationTest.java b/core/src/test/java/io/substrait/plan/PlanValidationTest.java index b34273b85..906074ed6 100644 --- a/core/src/test/java/io/substrait/plan/PlanValidationTest.java +++ b/core/src/test/java/io/substrait/plan/PlanValidationTest.java @@ -3,7 +3,6 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.Test; @@ -12,16 +11,6 @@ */ class PlanValidationTest { - private final ProtoPlanConverter fromProtoConverter = new ProtoPlanConverter(); - - private static io.substrait.proto.Version protoVersion(int major, int minor, int patch) { - return io.substrait.proto.Version.newBuilder() - .setMajorNumber(major) - .setMinorNumber(minor) - .setPatchNumber(patch) - .build(); - } - /** * Test case 1: Valid execution behavior with proper variable evaluation mode. * @@ -128,107 +117,4 @@ void testUnspecifiedVariableEvaluationMode() { exception.getMessage(), "Error message should indicate VariableEvaluationMode cannot be UNSPECIFIED"); } - - /** - * Test case 4: Missing execution behavior on a pre-0.87.0 plan defaults to PER_PLAN. - * - *

ExecutionBehavior did not exist before Substrait 0.87.0, so converting a protobuf Plan that - * explicitly declares an older version and omits execution behavior should default the variable - * evaluation mode to PER_PLAN rather than failing validation. - */ - @Test - void testMissingExecutionBehaviorOnOldVersionDefaultsToPerPlan() { - io.substrait.proto.Plan protoPlan = - io.substrait.proto.Plan.newBuilder().setVersion(protoVersion(0, 86, 0)).build(); - - Plan plan = assertDoesNotThrow(() -> fromProtoConverter.from(protoPlan)); - - assertTrue( - plan.getExecutionBehavior().isPresent(), - "Pre-0.87.0 plan should have a defaulted ExecutionBehavior"); - assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, - plan.getExecutionBehavior().get().getVariableEvaluationMode(), - "Pre-0.87.0 plan should default to PER_PLAN"); - } - - /** - * Test case 5: Missing execution behavior at exactly 0.87.0 still fails validation. - * - *

ExecutionBehavior is required from Substrait 0.87.0 onward, so a plan at that version with - * no execution behavior must fail validation rather than being defaulted. - */ - @Test - void testMissingExecutionBehaviorAtCutoffVersionFails() { - io.substrait.proto.Plan protoPlan = - io.substrait.proto.Plan.newBuilder().setVersion(protoVersion(0, 87, 0)).build(); - - IllegalArgumentException exception = - assertThrows(IllegalArgumentException.class, () -> fromProtoConverter.from(protoPlan)); - assertEquals( - "ExecutionBehavior is required but was not set", - exception.getMessage(), - "Plans at or after 0.87.0 require ExecutionBehavior"); - } - - /** - * Test case 6: Missing execution behavior on a newer plan still fails validation. - * - *

Verifies the requirement also holds for versions beyond the 0.87.0 cutoff. - */ - @Test - void testMissingExecutionBehaviorOnNewerVersionFails() { - io.substrait.proto.Plan protoPlan = - io.substrait.proto.Plan.newBuilder().setVersion(protoVersion(1, 0, 0)).build(); - - assertThrows(IllegalArgumentException.class, () -> fromProtoConverter.from(protoPlan)); - } - - /** - * Test case 7: A version-less plan reads as the protobuf default 0.0.0. - * - *

A protobuf Plan that does not declare a version at all has the default version 0.0.0, which - * is older than 0.87.0, so a missing execution behavior is defaulted to PER_PLAN. - */ - @Test - void testMissingExecutionBehaviorWithoutVersionDefaultsToPerPlan() { - io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); - - Plan plan = assertDoesNotThrow(() -> fromProtoConverter.from(protoPlan)); - - assertTrue( - plan.getExecutionBehavior().isPresent(), - "Version-less plan (0.0.0) should have a defaulted ExecutionBehavior"); - assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, - plan.getExecutionBehavior().get().getVariableEvaluationMode(), - "Version-less plan (0.0.0) should default to PER_PLAN"); - } - - /** - * Test case 8: An explicit execution behavior on an old plan is not overridden. - * - *

When a pre-0.87.0 plan does provide an execution behavior, the provided value must be used - * instead of the PER_PLAN default. - */ - @Test - void testExplicitExecutionBehaviorOnOldVersionIsPreserved() { - io.substrait.proto.Plan protoPlan = - io.substrait.proto.Plan.newBuilder() - .setVersion(protoVersion(0, 86, 0)) - .setExecutionBehavior( - io.substrait.proto.ExecutionBehavior.newBuilder() - .setVariableEvalMode( - io.substrait.proto.ExecutionBehavior.VariableEvaluationMode - .VARIABLE_EVALUATION_MODE_PER_RECORD) - .build()) - .build(); - - Plan plan = fromProtoConverter.from(protoPlan); - - assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD, - plan.getExecutionBehavior().get().getVariableEvaluationMode(), - "Explicit execution behavior should not be overridden by the default"); - } } From 92030779fb8b46d9f748567615d31ecbdbc8f035 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Mon, 8 Jun 2026 10:32:41 +0200 Subject: [PATCH 13/19] fix: always set default execution behavior when absent Signed-off-by: Niels Pardon --- .../io/substrait/plan/ProtoPlanConverter.java | 8 ++++ .../io/substrait/plan/PlanConverterTest.java | 38 ++++++++++++------- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java index b23f9e21c..eaf48cbf3 100644 --- a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java +++ b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java @@ -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; @@ -128,6 +130,12 @@ public Plan from(io.substrait.proto.Plan plan) { // Set execution behavior if present if (plan.hasExecutionBehavior()) { planBuilder.executionBehavior(fromProtoExecutionBehavior(plan.getExecutionBehavior())); + } else { + // set default ExecutionBehavior for older plans + planBuilder.executionBehavior( + ExecutionBehavior.builder() + .variableEvaluationMode(VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .build()); } return planBuilder.build(); diff --git a/core/src/test/java/io/substrait/plan/PlanConverterTest.java b/core/src/test/java/io/substrait/plan/PlanConverterTest.java index 0c2a7f9ba..6c00f3254 100644 --- a/core/src/test/java/io/substrait/plan/PlanConverterTest.java +++ b/core/src/test/java/io/substrait/plan/PlanConverterTest.java @@ -475,19 +475,24 @@ void testFromProtoWithExecutionBehaviorPerRecord() { /** * Conversion from protobuf Plan without ExecutionBehavior. * - *

Verifies that a protobuf Plan without ExecutionBehavior results in a POJO Plan that fails - * validation (since ExecutionBehavior is required). + *

Verifies that a protobuf Plan without ExecutionBehavior results in a POJO Plan with a default + * ExecutionBehavior of PER_PLAN, supporting older plans that predate the field. */ @Test - void testFromProtoWithoutExecutionBehaviorFailsValidation() { + void testFromProtoWithoutExecutionBehaviorUsesDefault() { // Create a protobuf Plan without ExecutionBehavior io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); - // Attempt to convert to POJO - should fail validation - assertThrows( - IllegalArgumentException.class, - () -> fromProtoConverter.from(protoPlan), - "Conversion should fail when ExecutionBehavior is not set"); + // Convert to POJO - should succeed with a default ExecutionBehavior + Plan plan = fromProtoConverter.from(protoPlan); + + assertTrue( + plan.getExecutionBehavior().isPresent(), + "Plan should have a default ExecutionBehavior present"); + assertEquals( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, + plan.getExecutionBehavior().get().getVariableEvaluationMode(), + "Default variable evaluation mode should be PER_PLAN"); } /** @@ -637,7 +642,7 @@ void testRoundTripEmptyPlanWithExecutionBehavior() { * Verify that protobuf without execution behavior field is handled. * *

Tests the edge case where a protobuf Plan doesn't have the execution behavior field set at - * all. + * all. Such older plans receive a default ExecutionBehavior of PER_PLAN. */ @Test void testFromProtoMissingExecutionBehaviorField() { @@ -648,10 +653,15 @@ void testFromProtoMissingExecutionBehaviorField() { assertFalse( protoPlan.hasExecutionBehavior(), "Protobuf Plan should not have execution behavior field"); - // Conversion should fail validation - assertThrows( - IllegalArgumentException.class, - () -> fromProtoConverter.from(protoPlan), - "Conversion should fail when execution behavior is missing"); + // Conversion should succeed with a default ExecutionBehavior of PER_PLAN + Plan plan = fromProtoConverter.from(protoPlan); + + assertTrue( + plan.getExecutionBehavior().isPresent(), + "Plan should have a default ExecutionBehavior present"); + assertEquals( + Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, + plan.getExecutionBehavior().get().getVariableEvaluationMode(), + "Default variable evaluation mode should be PER_PLAN"); } } From eef5ab526288f079d01604390b0c52fc46692b3d Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Mon, 8 Jun 2026 10:41:15 +0200 Subject: [PATCH 14/19] style: fix spotless Signed-off-by: Niels Pardon --- core/src/test/java/io/substrait/plan/PlanConverterTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/io/substrait/plan/PlanConverterTest.java b/core/src/test/java/io/substrait/plan/PlanConverterTest.java index 6c00f3254..4d0a98325 100644 --- a/core/src/test/java/io/substrait/plan/PlanConverterTest.java +++ b/core/src/test/java/io/substrait/plan/PlanConverterTest.java @@ -475,8 +475,8 @@ void testFromProtoWithExecutionBehaviorPerRecord() { /** * Conversion from protobuf Plan without ExecutionBehavior. * - *

Verifies that a protobuf Plan without ExecutionBehavior results in a POJO Plan with a default - * ExecutionBehavior of PER_PLAN, supporting older plans that predate the field. + *

Verifies that a protobuf Plan without ExecutionBehavior results in a POJO Plan with a + * default ExecutionBehavior of PER_PLAN, supporting older plans that predate the field. */ @Test void testFromProtoWithoutExecutionBehaviorUsesDefault() { From 475183b12372cb68d5970d8924496363801e8c68 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Tue, 9 Jun 2026 08:54:43 +0200 Subject: [PATCH 15/19] fix: address feedback from vbarua Signed-off-by: Niels Pardon --- .../src/main/java/io/substrait/plan/Plan.java | 7 ++-- .../io/substrait/plan/PlanProtoConverter.java | 6 ++-- .../io/substrait/plan/ProtoPlanConverter.java | 17 +++------- .../io/substrait/plan/PlanConverterTest.java | 34 ++++++++----------- .../io/substrait/plan/PlanValidationTest.java | 20 +++++------ 5 files changed, 33 insertions(+), 51 deletions(-) diff --git a/core/src/main/java/io/substrait/plan/Plan.java b/core/src/main/java/io/substrait/plan/Plan.java index a3f897632..57f4e9a49 100644 --- a/core/src/main/java/io/substrait/plan/Plan.java +++ b/core/src/main/java/io/substrait/plan/Plan.java @@ -21,7 +21,7 @@ public Version getVersion() { public abstract Optional getAdvancedExtension(); - public abstract Optional getExecutionBehavior(); + public abstract ExecutionBehavior getExecutionBehavior(); /** * Validates that the execution behavior is properly configured. @@ -41,10 +41,7 @@ public Version getVersion() { */ @Value.Check protected void check() { - if (!getExecutionBehavior().isPresent()) { - throw new IllegalArgumentException("ExecutionBehavior is required but was not set"); - } - ExecutionBehavior behavior = getExecutionBehavior().get(); + ExecutionBehavior behavior = getExecutionBehavior(); if (behavior.getVariableEvaluationMode() == ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_UNSPECIFIED) { throw new IllegalArgumentException( diff --git a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java index 5bb61749d..48c9cf30d 100644 --- a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java +++ b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java @@ -106,10 +106,8 @@ public Plan toProto(final io.substrait.plan.Plan plan) { builder.setVersion(versionBuilder); - // Set execution behavior if present - if (plan.getExecutionBehavior().isPresent()) { - builder.setExecutionBehavior(toProtoExecutionBehavior(plan.getExecutionBehavior().get())); - } + // Set execution behavior + builder.setExecutionBehavior(toProtoExecutionBehavior(plan.getExecutionBehavior())); return builder.build(); } diff --git a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java index eaf48cbf3..5bb4ee859 100644 --- a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java +++ b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java @@ -74,16 +74,9 @@ protected ProtoRelConverter getProtoRelConverter(final ExtensionLookup functionL *

Note: 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: * - *

    - *
  • For plans older than Substrait 0.87.0 (the version that introduced execution behavior), a - * missing execution behavior is defaulted to {@link - * io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_PER_PLAN}. - * Note that a plan without a version is treated as version 0.0.0, and therefore as older - * than 0.87.0. - *
  • For plans at or after 0.87.0, a missing execution behavior is left unset and Plan - * validation will fail. The validation also fails if the execution behavior is present but - * contains {@code VARIABLE_EVALUATION_MODE_UNSPECIFIED}. - *
+ *

Note: 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 @@ -127,11 +120,11 @@ public Plan from(io.substrait.proto.Plan plan) { : null)) .version(versionBuilder.build()); - // Set execution behavior if present + // Set execution behavior (required field) if (plan.hasExecutionBehavior()) { planBuilder.executionBehavior(fromProtoExecutionBehavior(plan.getExecutionBehavior())); } else { - // set default ExecutionBehavior for older plans + // Set default ExecutionBehavior for older plans that don't have it planBuilder.executionBehavior( ExecutionBehavior.builder() .variableEvaluationMode(VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) diff --git a/core/src/test/java/io/substrait/plan/PlanConverterTest.java b/core/src/test/java/io/substrait/plan/PlanConverterTest.java index 4d0a98325..8e71d80f9 100644 --- a/core/src/test/java/io/substrait/plan/PlanConverterTest.java +++ b/core/src/test/java/io/substrait/plan/PlanConverterTest.java @@ -434,11 +434,10 @@ void testFromProtoWithExecutionBehaviorPerPlan() { Plan plan = fromProtoConverter.from(protoPlan); // Verify the POJO has execution behavior - assertTrue( - plan.getExecutionBehavior().isPresent(), "Plan should have ExecutionBehavior present"); + assertNotNull(plan.getExecutionBehavior(), "Plan should have ExecutionBehavior"); assertEquals( Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, - plan.getExecutionBehavior().get().getVariableEvaluationMode(), + plan.getExecutionBehavior().getVariableEvaluationMode(), "Variable evaluation mode should be PER_PLAN"); } @@ -464,11 +463,10 @@ void testFromProtoWithExecutionBehaviorPerRecord() { Plan plan = fromProtoConverter.from(protoPlan); // Verify the POJO has execution behavior - assertTrue( - plan.getExecutionBehavior().isPresent(), "Plan should have ExecutionBehavior present"); + assertNotNull(plan.getExecutionBehavior(), "Plan should have ExecutionBehavior"); assertEquals( Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD, - plan.getExecutionBehavior().get().getVariableEvaluationMode(), + plan.getExecutionBehavior().getVariableEvaluationMode(), "Variable evaluation mode should be PER_RECORD"); } @@ -486,12 +484,10 @@ void testFromProtoWithoutExecutionBehaviorUsesDefault() { // Convert to POJO - should succeed with a default ExecutionBehavior Plan plan = fromProtoConverter.from(protoPlan); - assertTrue( - plan.getExecutionBehavior().isPresent(), - "Plan should have a default ExecutionBehavior present"); + assertNotNull(plan.getExecutionBehavior(), "Plan should have a default ExecutionBehavior"); assertEquals( Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, - plan.getExecutionBehavior().get().getVariableEvaluationMode(), + plan.getExecutionBehavior().getVariableEvaluationMode(), "Default variable evaluation mode should be PER_PLAN"); } @@ -553,12 +549,12 @@ void testRoundTripWithExecutionBehaviorPerPlan() { Plan roundTrippedPlan = fromProtoConverter.from(protoPlan); // Verify data integrity - assertTrue( - roundTrippedPlan.getExecutionBehavior().isPresent(), + assertNotNull( + roundTrippedPlan.getExecutionBehavior(), "Round-tripped Plan should have ExecutionBehavior"); assertEquals( Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, - roundTrippedPlan.getExecutionBehavior().get().getVariableEvaluationMode(), + roundTrippedPlan.getExecutionBehavior().getVariableEvaluationMode(), "Variable evaluation mode should be preserved"); assertEquals( originalPlan.getRoots().size(), @@ -597,12 +593,12 @@ void testRoundTripWithExecutionBehaviorPerRecord() { Plan roundTrippedPlan = fromProtoConverter.from(protoPlan); // Verify data integrity - assertTrue( - roundTrippedPlan.getExecutionBehavior().isPresent(), + assertNotNull( + roundTrippedPlan.getExecutionBehavior(), "Round-tripped Plan should have ExecutionBehavior"); assertEquals( Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD, - roundTrippedPlan.getExecutionBehavior().get().getVariableEvaluationMode(), + roundTrippedPlan.getExecutionBehavior().getVariableEvaluationMode(), "Variable evaluation mode should be preserved"); } @@ -656,12 +652,10 @@ void testFromProtoMissingExecutionBehaviorField() { // Conversion should succeed with a default ExecutionBehavior of PER_PLAN Plan plan = fromProtoConverter.from(protoPlan); - assertTrue( - plan.getExecutionBehavior().isPresent(), - "Plan should have a default ExecutionBehavior present"); + assertNotNull(plan.getExecutionBehavior(), "Plan should have a default ExecutionBehavior"); assertEquals( Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, - plan.getExecutionBehavior().get().getVariableEvaluationMode(), + plan.getExecutionBehavior().getVariableEvaluationMode(), "Default variable evaluation mode should be PER_PLAN"); } } diff --git a/core/src/test/java/io/substrait/plan/PlanValidationTest.java b/core/src/test/java/io/substrait/plan/PlanValidationTest.java index 906074ed6..b8698023d 100644 --- a/core/src/test/java/io/substrait/plan/PlanValidationTest.java +++ b/core/src/test/java/io/substrait/plan/PlanValidationTest.java @@ -3,6 +3,7 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.Test; @@ -59,28 +60,27 @@ void testValidExecutionBehavior_PerRecord() { } /** - * Test case 2: Missing execution behavior (empty Optional). + * Test case 2: Missing execution behavior. * *

This test verifies that attempting to create a Plan without setting the ExecutionBehavior - * field throws an IllegalArgumentException with an appropriate error message indicating that - * ExecutionBehavior is required. + * field throws an IllegalStateException (from Immutables) with an appropriate error message + * indicating that ExecutionBehavior is required. */ @Test void testMissingExecutionBehavior() { // Attempt to create a Plan without ExecutionBehavior - IllegalArgumentException exception = + IllegalStateException exception = assertThrows( - IllegalArgumentException.class, + IllegalStateException.class, () -> { ImmutablePlan.builder().build(); }, "Plan creation should fail when ExecutionBehavior is not set"); - // Verify the error message - assertEquals( - "ExecutionBehavior is required but was not set", - exception.getMessage(), - "Error message should indicate ExecutionBehavior is required"); + // Verify the error message mentions the required field + assertTrue( + exception.getMessage().contains("executionBehavior"), + "Error message should mention executionBehavior field"); } /** From c32e3d087cacee4150f4a68baf218db603408205 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Tue, 9 Jun 2026 08:59:29 +0200 Subject: [PATCH 16/19] docs: fix javadoc typo Signed-off-by: Niels Pardon --- core/src/main/java/io/substrait/plan/ProtoPlanConverter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java index 5bb4ee859..77edab7bf 100644 --- a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java +++ b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java @@ -74,7 +74,7 @@ protected ProtoRelConverter getProtoRelConverter(final ExtensionLookup functionL *

Note: 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: * - *

Note: If {@Code Plan.ExecutionBehavior.VariableEvaluationMode} is not set, it will be + *

Note: 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. * From 60baaa77bf85f09a6f1ed885f5420307f295b477 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Fri, 12 Jun 2026 14:41:01 +0200 Subject: [PATCH 17/19] fix: add enum proto mapping into enum Signed-off-by: Niels Pardon --- .../io/substrait/dsl/SubstraitBuilder.java | 6 +-- .../src/main/java/io/substrait/plan/Plan.java | 34 ++++++++++++---- .../io/substrait/plan/PlanProtoConverter.java | 40 +------------------ .../io/substrait/plan/ProtoPlanConverter.java | 11 ++--- .../io/substrait/plan/PlanConverterTest.java | 30 ++++++-------- .../io/substrait/plan/PlanValidationTest.java | 9 ++--- .../substrait/isthmus/ConverterProvider.java | 9 ++--- 7 files changed, 54 insertions(+), 85 deletions(-) diff --git a/core/src/main/java/io/substrait/dsl/SubstraitBuilder.java b/core/src/main/java/io/substrait/dsl/SubstraitBuilder.java index fc06c4932..37de1822c 100644 --- a/core/src/main/java/io/substrait/dsl/SubstraitBuilder.java +++ b/core/src/main/java/io/substrait/dsl/SubstraitBuilder.java @@ -1544,8 +1544,8 @@ public Plan.Root root(Rel rel) { /** * Creates a plan from a plan root with default execution behavior. * - *

The plan is created with {@link VariableEvaluationMode#VARIABLE_EVALUATION_MODE_PER_PLAN} as - * the default variable evaluation mode. To specify a custom execution behavior, use {@link + *

The plan is created with {@link VariableEvaluationMode#PER_PLAN} as the default variable + * evaluation mode. To specify a custom execution behavior, use {@link * #plan(Plan.ExecutionBehavior, Plan.Root)} instead. * * @param root the plan root @@ -1554,7 +1554,7 @@ public Plan.Root root(Rel rel) { public Plan plan(Plan.Root root) { return plan( ImmutableExecutionBehavior.builder() - .variableEvaluationMode(VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .variableEvaluationMode(VariableEvaluationMode.PER_PLAN) .build(), root); } diff --git a/core/src/main/java/io/substrait/plan/Plan.java b/core/src/main/java/io/substrait/plan/Plan.java index 57f4e9a49..6ec1c9d3e 100644 --- a/core/src/main/java/io/substrait/plan/Plan.java +++ b/core/src/main/java/io/substrait/plan/Plan.java @@ -32,18 +32,17 @@ public Version getVersion() { *

  • The {@link ExecutionBehavior} field is present (not null or empty) - ExecutionBehavior is * a required field *
  • The {@link ExecutionBehavior.VariableEvaluationMode} is set to a valid value (not {@link - * ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_UNSPECIFIED}) + * ExecutionBehavior.VariableEvaluationMode#UNSPECIFIED}) * * * @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} + * evaluation mode is set to {@link ExecutionBehavior.VariableEvaluationMode#UNSPECIFIED} */ @Value.Check protected void check() { ExecutionBehavior behavior = getExecutionBehavior(); if (behavior.getVariableEvaluationMode() - == ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_UNSPECIFIED) { + == ExecutionBehavior.VariableEvaluationMode.UNSPECIFIED) { throw new IllegalArgumentException( "ExecutionBehavior requires a specified VariableEvaluationMode, but got: " + behavior.getVariableEvaluationMode()); @@ -108,9 +107,30 @@ public static ImmutableExecutionBehavior.Builder builder() { } public enum VariableEvaluationMode { - VARIABLE_EVALUATION_MODE_UNSPECIFIED, - VARIABLE_EVALUATION_MODE_PER_PLAN, - VARIABLE_EVALUATION_MODE_PER_RECORD + UNSPECIFIED( + io.substrait.proto.ExecutionBehavior.VariableEvaluationMode + .VARIABLE_EVALUATION_MODE_UNSPECIFIED), + PER_PLAN( + io.substrait.proto.ExecutionBehavior.VariableEvaluationMode + .VARIABLE_EVALUATION_MODE_PER_PLAN), + PER_RECORD( + io.substrait.proto.ExecutionBehavior.VariableEvaluationMode + .VARIABLE_EVALUATION_MODE_PER_RECORD); + + private final io.substrait.proto.ExecutionBehavior.VariableEvaluationMode proto; + + VariableEvaluationMode(io.substrait.proto.ExecutionBehavior.VariableEvaluationMode proto) { + this.proto = proto; + } + + /** + * Converts this variable evaluation mode to its protobuf representation. + * + * @return the protobuf representation + */ + public io.substrait.proto.ExecutionBehavior.VariableEvaluationMode toProto() { + return proto; + } } } } diff --git a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java index 48c9cf30d..ea54e6821 100644 --- a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java +++ b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java @@ -5,7 +5,6 @@ import io.substrait.extension.ExtensionProtoConverter; import io.substrait.extension.SimpleExtension.ExtensionCollection; import io.substrait.proto.ExecutionBehavior; -import io.substrait.proto.ExecutionBehavior.VariableEvaluationMode; import io.substrait.proto.Plan; import io.substrait.proto.PlanRel; import io.substrait.proto.Rel; @@ -125,44 +124,7 @@ public Plan toProto(final io.substrait.plan.Plan plan) { private ExecutionBehavior toProtoExecutionBehavior( final io.substrait.plan.Plan.ExecutionBehavior executionBehavior) { return ExecutionBehavior.newBuilder() - .setVariableEvalMode( - toProtoVariableEvaluationMode(executionBehavior.getVariableEvaluationMode())) + .setVariableEvalMode(executionBehavior.getVariableEvaluationMode().toProto()) .build(); } - - /** - * Converts a {@link io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode} to its - * protobuf representation. - * - *

    Supported modes: - * - *

      - *
    • {@link - * io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_UNSPECIFIED} - * - Unspecified mode (should be avoided in valid plans) - *
    • {@link - * io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_PER_PLAN} - * - Variables are evaluated once per plan execution - *
    • {@link - * io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_PER_RECORD} - * - Variables are evaluated for each record - *
    - * - * @param mode the VariableEvaluationMode to convert, must not be null - * @return the protobuf VariableEvaluationMode representation - * @throws IllegalArgumentException if the mode is unknown or not supported - */ - private VariableEvaluationMode toProtoVariableEvaluationMode( - final io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode mode) { - switch (mode) { - case VARIABLE_EVALUATION_MODE_UNSPECIFIED: - return VariableEvaluationMode.VARIABLE_EVALUATION_MODE_UNSPECIFIED; - case VARIABLE_EVALUATION_MODE_PER_PLAN: - return VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN; - case VARIABLE_EVALUATION_MODE_PER_RECORD: - return VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD; - default: - throw new IllegalArgumentException("Unknown VariableEvaluationMode: " + mode); - } - } } diff --git a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java index 77edab7bf..bd01b4365 100644 --- a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java +++ b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java @@ -127,7 +127,7 @@ public Plan from(io.substrait.proto.Plan plan) { // Set default ExecutionBehavior for older plans that don't have it planBuilder.executionBehavior( ExecutionBehavior.builder() - .variableEvaluationMode(VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .variableEvaluationMode(VariableEvaluationMode.PER_PLAN) .build()); } @@ -179,14 +179,11 @@ private io.substrait.plan.Plan.ExecutionBehavior fromProtoExecutionBehavior( 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; + return io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode.UNSPECIFIED; case VARIABLE_EVALUATION_MODE_PER_PLAN: - return io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode - .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 - .VARIABLE_EVALUATION_MODE_PER_RECORD; + return io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode.PER_RECORD; case UNRECOGNIZED: default: throw new IllegalArgumentException("Unknown VariableEvaluationMode: " + mode); diff --git a/core/src/test/java/io/substrait/plan/PlanConverterTest.java b/core/src/test/java/io/substrait/plan/PlanConverterTest.java index 8e71d80f9..a7f3853d4 100644 --- a/core/src/test/java/io/substrait/plan/PlanConverterTest.java +++ b/core/src/test/java/io/substrait/plan/PlanConverterTest.java @@ -30,8 +30,7 @@ class PlanConverterTest { private static Plan.ExecutionBehavior defaultExecutionBehavior() { return ImmutableExecutionBehavior.builder() - .variableEvaluationMode( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN) .build(); } @@ -354,8 +353,7 @@ void testToProtoWithExecutionBehaviorPerPlan() { // Create a Plan with ExecutionBehavior set to PER_PLAN Plan.ExecutionBehavior executionBehavior = ImmutableExecutionBehavior.builder() - .variableEvaluationMode( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN) .build(); Plan plan = @@ -388,8 +386,7 @@ void testToProtoWithExecutionBehaviorPerRecord() { // Create a Plan with ExecutionBehavior set to PER_RECORD Plan.ExecutionBehavior executionBehavior = ImmutableExecutionBehavior.builder() - .variableEvaluationMode( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD) + .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_RECORD) .build(); Plan plan = @@ -436,7 +433,7 @@ void testFromProtoWithExecutionBehaviorPerPlan() { // Verify the POJO has execution behavior assertNotNull(plan.getExecutionBehavior(), "Plan should have ExecutionBehavior"); assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, + Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN, plan.getExecutionBehavior().getVariableEvaluationMode(), "Variable evaluation mode should be PER_PLAN"); } @@ -465,7 +462,7 @@ void testFromProtoWithExecutionBehaviorPerRecord() { // Verify the POJO has execution behavior assertNotNull(plan.getExecutionBehavior(), "Plan should have ExecutionBehavior"); assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD, + Plan.ExecutionBehavior.VariableEvaluationMode.PER_RECORD, plan.getExecutionBehavior().getVariableEvaluationMode(), "Variable evaluation mode should be PER_RECORD"); } @@ -486,7 +483,7 @@ void testFromProtoWithoutExecutionBehaviorUsesDefault() { assertNotNull(plan.getExecutionBehavior(), "Plan should have a default ExecutionBehavior"); assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, + Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN, plan.getExecutionBehavior().getVariableEvaluationMode(), "Default variable evaluation mode should be PER_PLAN"); } @@ -533,8 +530,7 @@ void testRoundTripWithExecutionBehaviorPerPlan() { // Create original Plan Plan.ExecutionBehavior executionBehavior = ImmutableExecutionBehavior.builder() - .variableEvaluationMode( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN) .build(); Plan originalPlan = @@ -553,7 +549,7 @@ void testRoundTripWithExecutionBehaviorPerPlan() { roundTrippedPlan.getExecutionBehavior(), "Round-tripped Plan should have ExecutionBehavior"); assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, + Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN, roundTrippedPlan.getExecutionBehavior().getVariableEvaluationMode(), "Variable evaluation mode should be preserved"); assertEquals( @@ -577,8 +573,7 @@ void testRoundTripWithExecutionBehaviorPerRecord() { // Create original Plan Plan.ExecutionBehavior executionBehavior = ImmutableExecutionBehavior.builder() - .variableEvaluationMode( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD) + .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_RECORD) .build(); Plan originalPlan = @@ -597,7 +592,7 @@ void testRoundTripWithExecutionBehaviorPerRecord() { roundTrippedPlan.getExecutionBehavior(), "Round-tripped Plan should have ExecutionBehavior"); assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD, + Plan.ExecutionBehavior.VariableEvaluationMode.PER_RECORD, roundTrippedPlan.getExecutionBehavior().getVariableEvaluationMode(), "Variable evaluation mode should be preserved"); } @@ -613,8 +608,7 @@ void testRoundTripEmptyPlanWithExecutionBehavior() { // Create minimal Plan Plan.ExecutionBehavior executionBehavior = ImmutableExecutionBehavior.builder() - .variableEvaluationMode( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN) .build(); Plan originalPlan = @@ -654,7 +648,7 @@ void testFromProtoMissingExecutionBehaviorField() { assertNotNull(plan.getExecutionBehavior(), "Plan should have a default ExecutionBehavior"); assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN, + Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN, plan.getExecutionBehavior().getVariableEvaluationMode(), "Default variable evaluation mode should be PER_PLAN"); } diff --git a/core/src/test/java/io/substrait/plan/PlanValidationTest.java b/core/src/test/java/io/substrait/plan/PlanValidationTest.java index b8698023d..499b456e9 100644 --- a/core/src/test/java/io/substrait/plan/PlanValidationTest.java +++ b/core/src/test/java/io/substrait/plan/PlanValidationTest.java @@ -24,8 +24,7 @@ void testValidExecutionBehavior_PerPlan() { // Create a valid ExecutionBehavior with VARIABLE_EVALUATION_MODE_PER_PLAN Plan.ExecutionBehavior executionBehavior = ImmutableExecutionBehavior.builder() - .variableEvaluationMode( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN) .build(); // Create a Plan with the valid ExecutionBehavior @@ -47,8 +46,7 @@ void testValidExecutionBehavior_PerRecord() { // Create a valid ExecutionBehavior with VARIABLE_EVALUATION_MODE_PER_RECORD Plan.ExecutionBehavior executionBehavior = ImmutableExecutionBehavior.builder() - .variableEvaluationMode( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_RECORD) + .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_RECORD) .build(); // Create a Plan with the valid ExecutionBehavior @@ -95,8 +93,7 @@ void testUnspecifiedVariableEvaluationMode() { // Create an ExecutionBehavior with VARIABLE_EVALUATION_MODE_UNSPECIFIED Plan.ExecutionBehavior executionBehavior = ImmutableExecutionBehavior.builder() - .variableEvaluationMode( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_UNSPECIFIED) + .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.UNSPECIFIED) .build(); // Attempt to create a Plan with the invalid ExecutionBehavior diff --git a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java index 19c6c4637..0d9271817 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java +++ b/isthmus/src/main/java/io/substrait/isthmus/ConverterProvider.java @@ -158,8 +158,7 @@ public ConverterProvider( */ private static Plan.ExecutionBehavior createDefaultExecutionBehavior() { return ImmutableExecutionBehavior.builder() - .variableEvaluationMode( - Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN) .build(); } @@ -396,9 +395,9 @@ public TypeConverter getTypeConverter() { * Returns the execution behavior for plans created by this converter. * *

    The default execution behavior uses {@link - * Plan.ExecutionBehavior.VariableEvaluationMode#VARIABLE_EVALUATION_MODE_PER_PLAN}, which - * evaluates variables once per plan execution. This can be customized by providing a different - * execution behavior through the constructor. + * Plan.ExecutionBehavior.VariableEvaluationMode#PER_PLAN}, which evaluates variables once per + * plan execution. This can be customized by providing a different execution behavior through the + * constructor. * * @return the execution behavior to use when creating plans */ From 2e26e83d8c6a116368bfce5cdadd108543030990 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Fri, 12 Jun 2026 14:49:10 +0200 Subject: [PATCH 18/19] fix: scala and tests Signed-off-by: Niels Pardon --- core/src/test/java/io/substrait/plan/PlanValidationTest.java | 3 +-- .../main/scala/io/substrait/spark/logical/ToSubstraitRel.scala | 2 +- .../test/scala/io/substrait/spark/AggregateWithJoinSuite.scala | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/core/src/test/java/io/substrait/plan/PlanValidationTest.java b/core/src/test/java/io/substrait/plan/PlanValidationTest.java index 499b456e9..06c98d96d 100644 --- a/core/src/test/java/io/substrait/plan/PlanValidationTest.java +++ b/core/src/test/java/io/substrait/plan/PlanValidationTest.java @@ -107,8 +107,7 @@ void testUnspecifiedVariableEvaluationMode() { // Verify the error message contains the expected information String expectedMessage = - "ExecutionBehavior requires a specified VariableEvaluationMode, but got: " - + "VARIABLE_EVALUATION_MODE_UNSPECIFIED"; + "ExecutionBehavior requires a specified VariableEvaluationMode, but got: " + "UNSPECIFIED"; assertEquals( expectedMessage, exception.getMessage(), diff --git a/spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala b/spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala index 2d4d9fb88..207a2efd4 100644 --- a/spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala +++ b/spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala @@ -710,7 +710,7 @@ class ToSubstraitRel extends AbstractLogicalPlanVisitor with Logging { .build()) .executionBehavior( ImmutableExecutionBehavior.builder() - .variableEvaluationMode(VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .variableEvaluationMode(VariableEvaluationMode.PER_PLAN) .build()) .roots( Collections.singletonList( diff --git a/spark/src/test/scala/io/substrait/spark/AggregateWithJoinSuite.scala b/spark/src/test/scala/io/substrait/spark/AggregateWithJoinSuite.scala index a669eeedf..d005bcf90 100644 --- a/spark/src/test/scala/io/substrait/spark/AggregateWithJoinSuite.scala +++ b/spark/src/test/scala/io/substrait/spark/AggregateWithJoinSuite.scala @@ -244,7 +244,7 @@ class AggregateWithJoinSuite extends SparkFunSuite with SharedSparkSession with .addRoots(root) .executionBehavior( Plan.ExecutionBehavior.builder() - .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.VARIABLE_EVALUATION_MODE_PER_PLAN) + .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN) .build() ) .build() From 702fde496a45f77fede50a9845f52c83911e1c22 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Fri, 12 Jun 2026 15:18:46 +0200 Subject: [PATCH 19/19] test: consolidate testcases Signed-off-by: Niels Pardon --- .../io/substrait/plan/PlanConverterTest.java | 240 +----------------- .../io/substrait/plan/PlanValidationTest.java | 43 +--- 2 files changed, 26 insertions(+), 257 deletions(-) diff --git a/core/src/test/java/io/substrait/plan/PlanConverterTest.java b/core/src/test/java/io/substrait/plan/PlanConverterTest.java index a7f3853d4..aaf71d87c 100644 --- a/core/src/test/java/io/substrait/plan/PlanConverterTest.java +++ b/core/src/test/java/io/substrait/plan/PlanConverterTest.java @@ -1,6 +1,5 @@ package io.substrait.plan; -import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -23,6 +22,8 @@ import java.util.Arrays; import java.util.Collections; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; class PlanConverterTest { private final PlanProtoConverter toProtoConverter = new PlanProtoConverter(); @@ -342,131 +343,6 @@ void nestedUserDefinedTypesShareExtensionCollector() { assertEquals(plan, roundTrippedPlan, "Plan should roundtrip correctly"); } - /** - * Conversion of Plan with VARIABLE_EVALUATION_MODE_PER_PLAN to protobuf. - * - *

    Verifies that a Plan with ExecutionBehavior set to PER_PLAN mode is correctly converted to - * protobuf format, including the execution behavior field. - */ - @Test - void testToProtoWithExecutionBehaviorPerPlan() { - // Create a Plan with ExecutionBehavior set to PER_PLAN - Plan.ExecutionBehavior executionBehavior = - ImmutableExecutionBehavior.builder() - .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN) - .build(); - - Plan plan = - ImmutablePlan.builder() - .executionBehavior(executionBehavior) - .roots(Collections.emptyList()) - .expectedTypeUrls(Collections.emptyList()) - .build(); - - // Convert to protobuf - io.substrait.proto.Plan protoPlan = toProtoConverter.toProto(plan); - - // Verify the protobuf has execution behavior - assertTrue(protoPlan.hasExecutionBehavior(), "Protobuf Plan should have ExecutionBehavior"); - assertEquals( - io.substrait.proto.ExecutionBehavior.VariableEvaluationMode - .VARIABLE_EVALUATION_MODE_PER_PLAN, - protoPlan.getExecutionBehavior().getVariableEvalMode(), - "Variable evaluation mode should be PER_PLAN"); - } - - /** - * Conversion of Plan with VARIABLE_EVALUATION_MODE_PER_RECORD to protobuf. - * - *

    Verifies that a Plan with ExecutionBehavior set to PER_RECORD mode is correctly converted to - * protobuf format. - */ - @Test - void testToProtoWithExecutionBehaviorPerRecord() { - // Create a Plan with ExecutionBehavior set to PER_RECORD - Plan.ExecutionBehavior executionBehavior = - ImmutableExecutionBehavior.builder() - .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_RECORD) - .build(); - - Plan plan = - ImmutablePlan.builder() - .executionBehavior(executionBehavior) - .roots(Collections.emptyList()) - .expectedTypeUrls(Collections.emptyList()) - .build(); - - // Convert to protobuf - io.substrait.proto.Plan protoPlan = toProtoConverter.toProto(plan); - - // Verify the protobuf has execution behavior - assertTrue(protoPlan.hasExecutionBehavior(), "Protobuf Plan should have ExecutionBehavior"); - assertEquals( - io.substrait.proto.ExecutionBehavior.VariableEvaluationMode - .VARIABLE_EVALUATION_MODE_PER_RECORD, - protoPlan.getExecutionBehavior().getVariableEvalMode(), - "Variable evaluation mode should be PER_RECORD"); - } - - /** - * Conversion from protobuf Plan with ExecutionBehavior to POJO. - * - *

    Verifies that a protobuf Plan with ExecutionBehavior is correctly converted to the POJO - * representation. - */ - @Test - void testFromProtoWithExecutionBehaviorPerPlan() { - // Create a protobuf Plan with ExecutionBehavior - io.substrait.proto.Plan protoPlan = - io.substrait.proto.Plan.newBuilder() - .setExecutionBehavior( - io.substrait.proto.ExecutionBehavior.newBuilder() - .setVariableEvalMode( - io.substrait.proto.ExecutionBehavior.VariableEvaluationMode - .VARIABLE_EVALUATION_MODE_PER_PLAN) - .build()) - .build(); - - // Convert to POJO - Plan plan = fromProtoConverter.from(protoPlan); - - // Verify the POJO has execution behavior - assertNotNull(plan.getExecutionBehavior(), "Plan should have ExecutionBehavior"); - assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN, - plan.getExecutionBehavior().getVariableEvaluationMode(), - "Variable evaluation mode should be PER_PLAN"); - } - - /** - * Conversion from protobuf Plan with PER_RECORD mode to POJO. - * - *

    Verifies that a protobuf Plan with PER_RECORD execution behavior is correctly converted. - */ - @Test - void testFromProtoWithExecutionBehaviorPerRecord() { - // Create a protobuf Plan with ExecutionBehavior set to PER_RECORD - io.substrait.proto.Plan protoPlan = - io.substrait.proto.Plan.newBuilder() - .setExecutionBehavior( - io.substrait.proto.ExecutionBehavior.newBuilder() - .setVariableEvalMode( - io.substrait.proto.ExecutionBehavior.VariableEvaluationMode - .VARIABLE_EVALUATION_MODE_PER_RECORD) - .build()) - .build(); - - // Convert to POJO - Plan plan = fromProtoConverter.from(protoPlan); - - // Verify the POJO has execution behavior - assertNotNull(plan.getExecutionBehavior(), "Plan should have ExecutionBehavior"); - assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.PER_RECORD, - plan.getExecutionBehavior().getVariableEvaluationMode(), - "Variable evaluation mode should be PER_RECORD"); - } - /** * Conversion from protobuf Plan without ExecutionBehavior. * @@ -477,6 +353,8 @@ void testFromProtoWithExecutionBehaviorPerRecord() { void testFromProtoWithoutExecutionBehaviorUsesDefault() { // Create a protobuf Plan without ExecutionBehavior io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); + assertFalse( + protoPlan.hasExecutionBehavior(), "Protobuf Plan should not have execution behavior field"); // Convert to POJO - should succeed with a default ExecutionBehavior Plan plan = fromProtoConverter.from(protoPlan); @@ -489,7 +367,7 @@ void testFromProtoWithoutExecutionBehaviorUsesDefault() { } /** - * Test case 6: Conversion from protobuf Plan with UNSPECIFIED mode fails validation. + * Conversion from protobuf Plan with UNSPECIFIED mode fails validation. * *

    Verifies that a protobuf Plan with VARIABLE_EVALUATION_MODE_UNSPECIFIED results in a * validation failure when converted to POJO. @@ -520,18 +398,19 @@ void testFromProtoWithUnspecifiedModeFailsValidation() { } /** - * Round-trip conversion with PER_PLAN mode. + * Round-trip conversion preserves the execution behavior. * *

    Verifies that converting a Plan to protobuf and back preserves all data, including the - * execution behavior with PER_PLAN mode. + * execution behavior, for every valid {@link Plan.ExecutionBehavior.VariableEvaluationMode}. */ - @Test - void testRoundTripWithExecutionBehaviorPerPlan() { + @ParameterizedTest + @EnumSource( + value = Plan.ExecutionBehavior.VariableEvaluationMode.class, + names = {"PER_PLAN", "PER_RECORD"}) + void testRoundTripWithExecutionBehavior(Plan.ExecutionBehavior.VariableEvaluationMode mode) { // Create original Plan Plan.ExecutionBehavior executionBehavior = - ImmutableExecutionBehavior.builder() - .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN) - .build(); + ImmutableExecutionBehavior.builder().variableEvaluationMode(mode).build(); Plan originalPlan = ImmutablePlan.builder() @@ -549,7 +428,7 @@ void testRoundTripWithExecutionBehaviorPerPlan() { roundTrippedPlan.getExecutionBehavior(), "Round-tripped Plan should have ExecutionBehavior"); assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN, + mode, roundTrippedPlan.getExecutionBehavior().getVariableEvaluationMode(), "Variable evaluation mode should be preserved"); assertEquals( @@ -561,95 +440,4 @@ void testRoundTripWithExecutionBehaviorPerPlan() { roundTrippedPlan.getExpectedTypeUrls().size(), "Number of expected type URLs should be preserved"); } - - /** - * Round-trip conversion with PER_RECORD mode. - * - *

    Verifies that converting a Plan with PER_RECORD mode to protobuf and back preserves the - * execution behavior correctly. - */ - @Test - void testRoundTripWithExecutionBehaviorPerRecord() { - // Create original Plan - Plan.ExecutionBehavior executionBehavior = - ImmutableExecutionBehavior.builder() - .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_RECORD) - .build(); - - Plan originalPlan = - ImmutablePlan.builder() - .executionBehavior(executionBehavior) - .roots(Collections.emptyList()) - .expectedTypeUrls(Collections.emptyList()) - .build(); - - // Convert to protobuf and back - io.substrait.proto.Plan protoPlan = toProtoConverter.toProto(originalPlan); - Plan roundTrippedPlan = fromProtoConverter.from(protoPlan); - - // Verify data integrity - assertNotNull( - roundTrippedPlan.getExecutionBehavior(), - "Round-tripped Plan should have ExecutionBehavior"); - assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.PER_RECORD, - roundTrippedPlan.getExecutionBehavior().getVariableEvaluationMode(), - "Variable evaluation mode should be preserved"); - } - - /** - * Empty plan with only execution behavior. - * - *

    Verifies that a minimal Plan with only execution behavior (no roots, no type URLs) can be - * successfully converted. - */ - @Test - void testRoundTripEmptyPlanWithExecutionBehavior() { - // Create minimal Plan - Plan.ExecutionBehavior executionBehavior = - ImmutableExecutionBehavior.builder() - .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN) - .build(); - - Plan originalPlan = - ImmutablePlan.builder() - .executionBehavior(executionBehavior) - .roots(Collections.emptyList()) - .expectedTypeUrls(Collections.emptyList()) - .build(); - - // Verify conversion succeeds - assertDoesNotThrow( - () -> { - io.substrait.proto.Plan protoPlan = toProtoConverter.toProto(originalPlan); - Plan roundTrippedPlan = fromProtoConverter.from(protoPlan); - assertNotNull(roundTrippedPlan, "Round-tripped Plan should not be null"); - }, - "Empty plan with execution behavior should convert successfully"); - } - - /** - * Verify that protobuf without execution behavior field is handled. - * - *

    Tests the edge case where a protobuf Plan doesn't have the execution behavior field set at - * all. Such older plans receive a default ExecutionBehavior of PER_PLAN. - */ - @Test - void testFromProtoMissingExecutionBehaviorField() { - // Create protobuf Plan without setting execution behavior - io.substrait.proto.Plan protoPlan = io.substrait.proto.Plan.newBuilder().build(); - - // Verify hasExecutionBehavior returns false - assertFalse( - protoPlan.hasExecutionBehavior(), "Protobuf Plan should not have execution behavior field"); - - // Conversion should succeed with a default ExecutionBehavior of PER_PLAN - Plan plan = fromProtoConverter.from(protoPlan); - - assertNotNull(plan.getExecutionBehavior(), "Plan should have a default ExecutionBehavior"); - assertEquals( - Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN, - plan.getExecutionBehavior().getVariableEvaluationMode(), - "Default variable evaluation mode should be PER_PLAN"); - } } diff --git a/core/src/test/java/io/substrait/plan/PlanValidationTest.java b/core/src/test/java/io/substrait/plan/PlanValidationTest.java index 06c98d96d..b90adff4c 100644 --- a/core/src/test/java/io/substrait/plan/PlanValidationTest.java +++ b/core/src/test/java/io/substrait/plan/PlanValidationTest.java @@ -6,6 +6,8 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; /** * Test cases for the Plan validation method that ensures ExecutionBehavior is properly configured. @@ -13,41 +15,20 @@ class PlanValidationTest { /** - * Test case 1: Valid execution behavior with proper variable evaluation mode. + * Valid execution behavior with a specified variable evaluation mode. * *

    This test verifies that a Plan with a properly configured ExecutionBehavior (with * VariableEvaluationMode set to a valid value other than UNSPECIFIED) is successfully created * without throwing any exceptions. */ - @Test - void testValidExecutionBehavior_PerPlan() { - // Create a valid ExecutionBehavior with VARIABLE_EVALUATION_MODE_PER_PLAN + @ParameterizedTest + @EnumSource( + value = Plan.ExecutionBehavior.VariableEvaluationMode.class, + names = {"PER_PLAN", "PER_RECORD"}) + void testValidExecutionBehavior(Plan.ExecutionBehavior.VariableEvaluationMode mode) { + // Create a valid ExecutionBehavior Plan.ExecutionBehavior executionBehavior = - ImmutableExecutionBehavior.builder() - .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_PLAN) - .build(); - - // Create a Plan with the valid ExecutionBehavior - assertDoesNotThrow( - () -> { - ImmutablePlan.builder().executionBehavior(executionBehavior).build(); - }, - "Plan creation should succeed with valid ExecutionBehavior"); - } - - /** - * Test case 1b: Valid execution behavior with VARIABLE_EVALUATION_MODE_PER_RECORD. - * - *

    This test verifies that a Plan with ExecutionBehavior set to - * VARIABLE_EVALUATION_MODE_PER_RECORD is also valid. - */ - @Test - void testValidExecutionBehavior_PerRecord() { - // Create a valid ExecutionBehavior with VARIABLE_EVALUATION_MODE_PER_RECORD - Plan.ExecutionBehavior executionBehavior = - ImmutableExecutionBehavior.builder() - .variableEvaluationMode(Plan.ExecutionBehavior.VariableEvaluationMode.PER_RECORD) - .build(); + ImmutableExecutionBehavior.builder().variableEvaluationMode(mode).build(); // Create a Plan with the valid ExecutionBehavior assertDoesNotThrow( @@ -58,7 +39,7 @@ void testValidExecutionBehavior_PerRecord() { } /** - * Test case 2: Missing execution behavior. + * Missing execution behavior. * *

    This test verifies that attempting to create a Plan without setting the ExecutionBehavior * field throws an IllegalStateException (from Immutables) with an appropriate error message @@ -82,7 +63,7 @@ void testMissingExecutionBehavior() { } /** - * Test case 3: Execution behavior with unspecified variable evaluation mode. + * Execution behavior with unspecified variable evaluation mode. * *

    This test verifies that attempting to create a Plan with an ExecutionBehavior that has its * VariableEvaluationMode set to VARIABLE_EVALUATION_MODE_UNSPECIFIED throws an