feat: enable support from/to pojo/protobuf for extended expressions #206

davisusanibar · 2023-11-24T21:58:44Z

To closes #127

danepitkin

This looks awesome! I took a first pass at the feature, but haven't reviewed the tests yet.

core/src/main/java/io/substrait/extended/expression/ExtendedExpression.java

danepitkin · 2023-11-28T18:53:53Z

core/src/main/java/io/substrait/extended/expression/ExtendedExpression.java

+
+  @Value.Immutable
+  public abstract static class ExpressionReference {
+    public abstract Expression getReferredExpr();


We should support AggregateFunction too according to the protobuf: https://github.com/substrait-io/substrait/blob/8f8d85e63501fc1a085d9585c6566e9c33b81264/proto/substrait/extended_expression.proto#L16C1-L23C2

I have added AggregateFunction and marked it as an UnsupportedOperationException for now.

core/src/main/java/io/substrait/extended/expression/ExtendedExpression.java

core/src/main/java/io/substrait/extended/expression/ExtendedExpressionProtoConverter.java

core/src/main/java/io/substrait/extended/expression/ProtoExtendedExpressionConverter.java

core/src/main/java/io/substrait/extension/ImmutableExtensionLookup.java

core/src/test/java/io/substrait/extended/expression/ProtoExtendedExpressionConverterTest.java

Co-authored-by: Dane Pitkin <[email protected]>

danepitkin

Thanks David! I left a few more comments

core/src/main/java/io/substrait/type/NamedStruct.java

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

danepitkin · 2023-11-30T22:06:33Z

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

+      } else if (expressionType
+          instanceof io.substrait.extendedexpression.ExtendedExpression.AggregateFunctionType) {
+        throw new UnsupportedOperationException(
+            "Aggregate function types are not supported in conversion to proto Extended Expressions for now");


Could we go ahead and implement in this PR? Would it be a lot of work?

Just added.

I think this actually may be a fair bit of work to do properly. In the interest of keeping this PR small and moving this work along, I think we could include AggregateFunction support as a future change.

What do you think @danepitkin

+1 I agree. I didn't realize the level of effort required here. Sorry, @davisusanibar !

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

core/src/test/java/io/substrait/extendedexpression/ExtendedExpressionProtoConverterTest.java

danepitkin · 2023-11-30T22:19:39Z

core/src/test/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverterTest.java

+  static final String NAMESPACE = "/functions_arithmetic_decimal.yaml";
+
+  @Test
+  public void fromTest() throws IOException {


It might be better to have a single roundtripExtendedExpression test so that you can test both to/from proto and compare the full objects afterwards (instead of toProtoTest and fromProtoTest).

Just maintain only roundtrip class for Expression, Aggregation and Expression + Aggregation

…o/proto

danepitkin

Nice work, @davisusanibar ! Left a few minor comments, but overall LGTM.

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

core/src/test/java/io/substrait/extendedexpression/ExtendedExpressionRoundTripTest.java

Co-authored-by: Dane Pitkin <[email protected]>

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

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

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

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

danepitkin · 2023-12-07T19:20:14Z

Thanks, Victor, for pointing out there is still some work to do to implement AggregateFunctions. It seems like it may be best to push AggregateFunction support to a follow up issue to keep this PR small.

davisusanibar · 2023-12-08T19:45:03Z

Thanks, Victor, for pointing out there is still some work to do to implement AggregateFunctions. It seems like it may be best to push AggregateFunction support to a follow up issue to keep this PR small.

Hi Dane, just doing my last attempt to try to cover AggregateFunction ... I could suggest that it is needed to have a uniform API that won't change that dramatically later.

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

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

…rter

danepitkin · 2023-12-12T22:49:30Z

I'll let @vbarua comment on if there is more work needed for AggregateFunctions, as I'm not yet that familiar with them. If there is more work required, we should do it in a follow up PR to keep this one small. The Expression support LGTM and I wouldn't want to hold up merging it, especially if the SQL -> ExtendedExpression PR would benefit from it!

vbarua

Thanks for adding this. I left some comments about things that we can improve in the future, but overall looks great.

I would like two thing before approving and merging:

Could you update the ProtoExtendedExpressionConverter to not use comparisons to getNumber and instead compare the protobuf enums.
Could you answer my question about the @Desugar annotation.

vbarua · 2023-12-13T00:45:20Z

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

+                .collect(java.util.stream.Collectors.toList()))
+        .setFunctionReference(
+            functionCollector.getFunctionReference(measure.getFunction().declaration()))
+        .build();


This duplicates code RelProtoConverter#toProto(Aggregate.Measure measure).

It's fine for now, but we should consider unifying it in the future.

I will create a ticket to address this issue.

vbarua · 2023-12-13T00:49:02Z

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

+                .aggregationPhase(Expression.AggregationPhase.fromProto(measure.getPhase()))
+                .invocation(Expression.AggregationInvocation.fromProto(measure.getInvocation()))
+                .build())
+        .build();


This duplicates code in ProtoRelConverter#newAggregate(AggregateRel rel).

It's fine for now but we should consider unifying it in the future.

I will create a ticket to address this issue.

vbarua · 2023-12-13T00:53:50Z

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

+    for (io.substrait.extendedexpression.ExtendedExpression.ExpressionReferenceBase
+        expressionReference : extendedExpression.getReferredExpressions()) {
+      if (expressionReference
+          instanceof io.substrait.extendedexpression.ExtendedExpression.ExpressionReference et) {


I'm still not a huge fan of these instanceof checks, but we can always improve this later.

I will create a ticket to address this issue.

core/src/main/java/io/substrait/extension/ExtensionCollector.java

vbarua · 2023-12-13T01:03:23Z

core/src/test/java/io/substrait/extendedexpression/ExtendedExpressionRoundTripTest.java

+  }
+
+  @ParameterizedTest
+  @MethodSource("expressionReferenceProvider")


TIL about @MethodSource. That's pretty nifty.

…otobuf enums

vbarua

Changes look good. Thanks for working with us on this @davisusanibar!

danepitkin · 2023-12-14T15:45:23Z

Thanks, @davisusanibar ! Nice work!!

davisusanibar added 2 commits November 24, 2023 16:56

feat: enable support from/to pojo/protobuf for extended expressions

75e4f48

Merge branch 'main' into feature/from_to_protobuf_pojo

1d23187

davisusanibar mentioned this pull request Nov 25, 2023

feat: convert sql expression into proto extended expressions #191

Merged

8 tasks

davisusanibar marked this pull request as ready for review November 25, 2023 00:02

This was referenced Nov 25, 2023

[Isthmus][CLI] Expose ExtendedExpression on Isthmus graal image using PicoCLI #207

Closed

[Core][Isthmus][CLI] Support extended expression definitions for SQL expressions #208

Open

vibhatha requested a review from vbarua November 25, 2023 09:34

danepitkin reviewed Nov 28, 2023

View reviewed changes

vbarua reviewed Nov 28, 2023

View reviewed changes

core/src/test/java/io/substrait/extended/expression/ProtoExtendedExpressionConverterTest.java Outdated Show resolved Hide resolved

davisusanibar and others added 3 commits November 29, 2023 19:06

fix: apply suggestions from code review

f817eb0

Co-authored-by: Dane Pitkin <[email protected]>

fix: code review core module

b1c96bd

fix: code review core module testing side

3d9b927

davisusanibar force-pushed the feature/from_to_protobuf_pojo branch from f05eedf to 3d9b927 Compare November 30, 2023 00:09

danepitkin reviewed Nov 30, 2023

View reviewed changes

feat: support aggregation function in extended expression from/to poj…

e790492

…o/proto

davisusanibar mentioned this pull request Dec 6, 2023

feat(isthmus): support for SQL expressions in CLI #209

Merged

davisusanibar requested review from danepitkin and vbarua December 6, 2023 15:41

danepitkin reviewed Dec 6, 2023

View reviewed changes

fix: code review suggestion

0fa69c8

Co-authored-by: Dane Pitkin <[email protected]>

vbarua reviewed Dec 7, 2023

View reviewed changes

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

refactor: bind instanceof checked variables

92d2cc5

vbarua reviewed Dec 7, 2023

View reviewed changes

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

vbarua reviewed Dec 7, 2023

View reviewed changes

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

vbarua reviewed Dec 7, 2023

View reviewed changes

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

vbarua reviewed Dec 7, 2023

View reviewed changes

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

davisusanibar added 2 commits December 8, 2023 10:58

fix: adding Aggregate.Measure POJO instead of Proto

379b83f

fix: simplify extended expression immutable class

e415785

fix: clean code

c27dd37

vbarua reviewed Dec 8, 2023

View reviewed changes

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

vbarua reviewed Dec 8, 2023

View reviewed changes

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

fix: support any kind of expression type on extended expression conve…

a5d8126

…rter

davisusanibar requested a review from vbarua December 9, 2023 08:51

fix: error scalar function test case

50602f2

vbarua reviewed Dec 13, 2023

View reviewed changes

fix: protoExtendedExpressionConverter to not use getNumber and use pr…

1c43b73

…otobuf enums

davisusanibar requested a review from vbarua December 13, 2023 19:20

vbarua approved these changes Dec 14, 2023

View reviewed changes

vbarua merged commit 9e023a7 into substrait-io:main Dec 14, 2023
8 checks passed

ajegou pushed a commit to ajegou/substrait-java that referenced this pull request Mar 29, 2024

feat(pojo): support for extended expressions (substrait-io#206)

9abf1a0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable support from/to pojo/protobuf for extended expressions #206

feat: enable support from/to pojo/protobuf for extended expressions #206

davisusanibar commented Nov 24, 2023

danepitkin left a comment

danepitkin Nov 28, 2023

davisusanibar Nov 29, 2023

danepitkin left a comment

danepitkin Nov 30, 2023

davisusanibar Dec 6, 2023

vbarua Dec 7, 2023

danepitkin Dec 7, 2023

danepitkin Nov 30, 2023

davisusanibar Dec 6, 2023

danepitkin left a comment

danepitkin commented Dec 7, 2023

davisusanibar commented Dec 8, 2023

danepitkin commented Dec 12, 2023

vbarua left a comment

vbarua Dec 13, 2023

davisusanibar Dec 13, 2023

vbarua Dec 13, 2023 •

edited

Loading

davisusanibar Dec 13, 2023

vbarua Dec 13, 2023

davisusanibar Dec 13, 2023

vbarua Dec 13, 2023

vbarua left a comment

danepitkin commented Dec 14, 2023

feat: enable support from/to pojo/protobuf for extended expressions #206

feat: enable support from/to pojo/protobuf for extended expressions #206

Conversation

davisusanibar commented Nov 24, 2023

danepitkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danepitkin 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

Choose a reason for hiding this comment

danepitkin left a comment

Choose a reason for hiding this comment

danepitkin commented Dec 7, 2023

davisusanibar commented Dec 8, 2023

danepitkin commented Dec 12, 2023

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

vbarua Dec 13, 2023 • 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

danepitkin commented Dec 14, 2023

vbarua Dec 13, 2023 •

edited

Loading