fix: properly support Options for Scalar/Aggregate/Window functions #278

Blizzara · 2024-07-02T12:20:30Z

Options were supported in the Java immutables but in most cases dropped when converting into protobuf

This

fixes the protobuf conversion,
adds a builder for FunctionOption
adds a ExpressionCreator functions with options
does some refactoring

Blizzara · 2024-07-02T12:21:12Z

core/src/main/java/io/substrait/expression/AggregateFunctionInvocation.java

@@ -12,7 +11,7 @@ public abstract class AggregateFunctionInvocation {

  public abstract List<FunctionArg> arguments();

-  public abstract Map<String, FunctionOption> options();


This seemed weird to me, since the name is already kept inside the FunctionOption, so this just duplicates the source of truth. That said I don't mind reverting this if it's preferable to keep the map.

I imagine this is/was intended to allow for easy lookup of options given a name, so that consumers could look up the relevant options for a given function. However given that:

// Name of the option to set. If the consumer does not recognize the // option, it must reject the plan. The name is matched case-insensitively // with option names defined for the function.

https://github.com/substrait-io/substrait/blob/7dbbf0468083d932a61b9c720700bd6083558fa9/proto/substrait/algebra.proto#L741-L744

It probably is safer to have this be a list and force consumers to process all of them.

Blizzara · 2024-07-02T12:22:36Z

core/src/main/java/io/substrait/expression/ExpressionCreator.java

  }

  public static Expression.ScalarFunctionInvocation scalarFunction(
      SimpleExtension.ScalarFunctionVariant declaration,
      Type outputType,
+      List<? extends FunctionOption> options,


I wasn't sure if it's better to change the existing functions or add new ones given this is an optional parameter, so I added new ones. But happy to remove the old ones too if that'd be better?

I think were getting close to the point where this pattern (individual constructor variants with different field combinations) may not be good as we start to explode the number of function combinations. These are fine for now, but if were to add more beyond these it might make sense to revisit this utility function approach.

That's fair - I decided to revert the additions and add some docstrings to point to using the builder directly in 9aae3ec, does that seem alright to you?

core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java

vbarua

Started taking a look. This seems fairly reasonable so far. I can spend some more time on this tomorrow.

vbarua · 2024-07-02T22:16:36Z

core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java

-        .name(o.getName())
-        .addAllValues(o.getPreferenceList())
-        .build();
+  public static FunctionOption fromFunctionOption(io.substrait.proto.FunctionOption o) {


It is somewhat awkward to make this static so it can be re-used in the ProtoAggregateFunctionConverter, but I also can't think of a better place to put this.

vbarua · 2024-07-02T22:16:50Z

core/src/main/java/io/substrait/expression/FunctionOption.java

+
+  public static ImmutableFunctionOption.Builder builder() {
+    return ImmutableFunctionOption.builder();
+  }


Good addition ✨

vbarua · 2024-07-02T22:26:57Z

core/src/main/java/io/substrait/expression/AggregateFunctionInvocation.java

@@ -12,7 +11,7 @@ public abstract class AggregateFunctionInvocation {

  public abstract List<FunctionArg> arguments();

-  public abstract Map<String, FunctionOption> options();


I imagine this is/was intended to allow for easy lookup of options given a name, so that consumers could look up the relevant options for a given function. However given that:

// Name of the option to set. If the consumer does not recognize the // option, it must reject the plan. The name is matched case-insensitively // with option names defined for the function.

https://github.com/substrait-io/substrait/blob/7dbbf0468083d932a61b9c720700bd6083558fa9/proto/substrait/algebra.proto#L741-L744

It probably is safer to have this be a list and force consumers to process all of them.

vbarua · 2024-07-02T22:35:39Z

core/src/test/java/io/substrait/type/proto/ConsistentPartitionWindowRelRoundtripTest.java

+                                FunctionOption.builder()
+                                    .name("option")
+                                    .addValues("VALUE1", "VALUE2")
+                                    .build()))


meta: The lead function doesn't actually define any options. Potentially, we should reject both:

Building plans w/ invalid options.

Reading protobuf plans in w/ invalid options.

I think this is outside the scope of this PR.

Fair 😅 but yep agreed it'd be a separate PR

Blizzara · 2024-07-05T15:10:31Z

@vbarua did you have a chance to give this another look? :)

Also, any ideas why the editorconfig check is complaining? The files it mentions seem unrelated to this PR:

Run editorconfig-checker
.gitmodules:
	2: Wrong indent style found (tabs instead of spaces)
	3: Wrong indent style found (tabs instead of spaces)
core/src/main/antlr/SubstraitType.g[4](https://github.com/substrait-io/substrait-java/actions/runs/9796466765/job/27050988486?pr=278#step:4:5):
	110: Wrong indent style found (tabs instead of spaces)
...

vbarua · 2024-07-05T19:53:43Z

any ideas why the editorconfig check is complaining?

Not only where they unrelated to your PR, they also haven't been modified in a long time. I've fixed the issues in:
#280

https://substrait.io/expressions/scalar_functions/#options Options were supported in the Java classes but in most cases dropped when converting into protobuf This - fixes the protobuf conversion, - adds a builder for FunctionOption - adds a ExpressionCreator functions with options - does some refactoring

vbarua

Overall this looks reasonable to me.

vbarua · 2024-07-08T21:25:41Z

core/src/main/java/io/substrait/relation/RelProtoConverter.java

-                                  .addAllPreference(o.getValue().values())
-                                  .build())
+                  f.options().stream()
+                      .map(ExpressionProtoConverter::from)


meta: I think re-using this is good, but also think it's a bit weird that this lives in ExpressionProtoConverter and is used in RelProtoConverter. I'm not sure what a better place for this is yet, so I think it's fine to keep this here for now.

vbarua · 2024-07-08T21:36:37Z

core/src/main/java/io/substrait/relation/ProtoAggregateFunctionConverter.java

+        .outputType(protoTypeConverter.from(measure.getOutputType()))
+        .aggregationPhase(Expression.AggregationPhase.fromProto(measure.getPhase()))
+        .invocation(Expression.AggregationInvocation.fromProto(measure.getInvocation()))
+        .options(options)
        .build();


The other advantage of this is that by forcing callers to wrap this in a Measure themselves, it also makes it easier for them to include the PreMeasure filter when it's available.

vbarua · 2024-07-08T21:44:48Z

core/src/main/java/io/substrait/expression/ExpressionCreator.java

  }

  public static Expression.ScalarFunctionInvocation scalarFunction(
      SimpleExtension.ScalarFunctionVariant declaration,
      Type outputType,
+      List<? extends FunctionOption> options,


I think were getting close to the point where this pattern (individual constructor variants with different field combinations) may not be good as we start to explode the number of function combinations. These are fine for now, but if were to add more beyond these it might make sense to revisit this utility function approach.

Blizzara commented Jul 2, 2024

View reviewed changes

core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java Show resolved Hide resolved

Blizzara force-pushed the avo/support-function-options branch from 984583b to 484c512 Compare July 2, 2024 12:24

vbarua reviewed Jul 2, 2024

View reviewed changes

Blizzara added 2 commits July 8, 2024 11:01

fix: spotlessApply

e786345

Blizzara force-pushed the avo/support-function-options branch from 92f800c to e786345 Compare July 8, 2024 09:01

vbarua approved these changes Jul 8, 2024

View reviewed changes

revert additional creators

9aae3ec

vbarua merged commit e574913 into substrait-io:main Jul 11, 2024
12 checks passed

Blizzara deleted the avo/support-function-options branch August 20, 2024 07:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: properly support Options for Scalar/Aggregate/Window functions #278

fix: properly support Options for Scalar/Aggregate/Window functions #278

Blizzara commented Jul 2, 2024

Blizzara Jul 2, 2024 •

edited

Loading

vbarua Jul 2, 2024

Blizzara Jul 2, 2024

vbarua Jul 8, 2024

Blizzara Jul 10, 2024

vbarua left a comment

vbarua Jul 2, 2024

vbarua Jul 2, 2024

vbarua Jul 2, 2024

vbarua Jul 2, 2024

Blizzara Jul 3, 2024

Blizzara commented Jul 5, 2024

vbarua commented Jul 5, 2024

vbarua left a comment

vbarua Jul 8, 2024

vbarua Jul 8, 2024

vbarua Jul 8, 2024

		@@ -12,7 +11,7 @@ public abstract class AggregateFunctionInvocation {

		public abstract List<FunctionArg> arguments();

		public abstract Map<String, FunctionOption> options();

fix: properly support Options for Scalar/Aggregate/Window functions #278

fix: properly support Options for Scalar/Aggregate/Window functions #278

Conversation

Blizzara commented Jul 2, 2024

Blizzara Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbarua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blizzara commented Jul 5, 2024

vbarua commented Jul 5, 2024

vbarua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blizzara Jul 2, 2024 •

edited

Loading