feat: add POJOs for ConsistentPartitionWindowRel and WindowRelFunction #231

bvolpato · 2024-02-14T02:09:40Z

Adding the POJO for ConsistentPartitionWindowRel and WindowRelFunction: https://github.com/substrait-io/substrait/blob/main/proto/substrait/algebra.proto#L233-L267

I wasn't sure about the organization/place of some parts since it's not physically under Expression, but there's a lot in common, even highlighted in the proto:

// This message mirrors the WindowFunction message but removes the fields defining the partition,
// sorts, and bounds, since those must be consistent across the various functions in this rel. Refer
// to the WindowFunction message for a description of these fields.

So I tried to keep it as a new base class, but tried to keep the converters as close as they were to make it more concise + simplifying reuse.

--

I'll have an Isthmus change following (this is essentially a cleanup / split of #228), and @vbarua rightfully suggested breaking it down.

…vocation

vbarua · 2024-02-15T04:42:46Z

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

+            t -> t.accept(getExpressionCopyOnWriteVisitor()));
+    var sorts = transformList(consistentPartitionWindow.getSorts(), this::visitSortField);
+
+    return Optional.of(


To follow the pattern of the other copy on write visitors, this should return empty unless any of the Optionals windowFunctions, partitionExpressions and sorts is non-empty.

vbarua

Missing a couple field conversions, but overall this looks good.

I wasn't sure about the organization/place of some parts since it's not physically under Expression, but there's a lot in common, even highlighted in the proto:

I can't a think of a alternate organization at this point either. Grouping the WindowRelFunctionInvocation conversion code alongside the equivalent WindowFunctionInvocation conversion code makes sense to me, and at least keeps it together. There might be some nicer abstraction we could use to unify these somehow, but I think that would be a bigger refactor and should be left for another time.

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

vbarua · 2024-02-15T04:47:18Z

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

+        ConsistentPartitionWindow.WindowRelFunctionInvocation.builder()
+            .arguments(
+                visitFunctionArguments(windowRelFunctionInvocation.arguments())
+                    .orElse(windowRelFunctionInvocation.arguments()))


This should return empty if visitFunctionArguments returns an empty optional to keep the copy on write behaviour.

Done, thanks!

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

vbarua · 2024-02-15T04:58:04Z

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

@@ -250,6 +219,80 @@ public Expression from(io.substrait.proto.Expression expr) {
    };
  }

+  public ImmutableExpression.WindowFunctionInvocation fromWindowFunction(


minor: We should return Expression.WindowFunctionInvocation here. We generally avoid using the Immutable generate classes in signatures.

Good call extracting this out. It's much easier to look at alongside fromWindowRelFunction like this.

Thanks! Overlook when using extract method on IntelliJ

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

vbarua · 2024-02-15T05:08:20Z

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

+        .collect(Collectors.toList());
+  }
+
+  public ImmutableExpression.SortField fromSortField(SortField s) {


minor: This should return Expression.SortField

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

vbarua · 2024-02-15T05:36:32Z

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

+  }
+
+  @Value.Immutable
+  public abstract static class WindowRelFunctionInvocation {


Not for this PR. It might make sense in the future to define an interface of common types that we can share between WindowRelFunctionInvocation and WindowFunctionInvocation.

Constructing a WindowRelFunctionInvocation from that interface would be straightforward (as it would have all the fields), and then construction a WindowRelFunctionInvocation would just required providing the partitions and sorts.

We might also be able to have WindowFunctionInvocation extend from WindowRelFunctionInvocation, but I vaguely recall trying something like that in the past and the Immutable library not liking it.

Yeah I agree -- it's subtly different and can be annoying to have concise builders and everything.

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

bvolpato · 2024-02-15T15:07:58Z

Thanks for the great review @vbarua 💯.

All good points! Pushed 1bb9836 addressing them.

…ields

…ipping

vbarua · 2024-02-16T21:52:36Z

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

@@ -377,14 +377,8 @@ protected Optional<ConsistentPartitionWindow.WindowRelFunctionInvocation> visitW

    return Optional.of(
        ConsistentPartitionWindow.WindowRelFunctionInvocation.builder()
+            .from(windowRelFunctionInvocation)


Using from here both:

saves use some lines of code

prevents issues if we ever add fields to the ConsistentPartitionWindow (as we would then need to make sure we updated this code to manually copy over those fields)

vbarua · 2024-02-16T21:53:23Z

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

-import java.util.Collections;
-import org.junit.jupiter.api.Test;
-
-public class WindowFunctionRoundtripTest extends TestBase {


Renamed to ConsistentPartitionWindowRelRoundtripTest as we're testing the roundtripability of the rel as a whole.

vbarua · 2024-02-16T21:55:36Z

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

+    var windowFunctionDeclaration =
+        defaultExtensionCollection.getWindowFunction(
+            SimpleExtension.FunctionAnchor.of(
+                DefaultExtensionCatalog.FUNCTIONS_ARITHMETIC, "lead:any"));


Choose a specific function (in this case lead:any) instead of a random one from defaultExtensionCollection.windowFunctions().get(0). There's no guarantee that a function chosen at random will be valid with the arguments that we choose to pass.

vbarua · 2024-02-16T21:55:57Z

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

+        b.namedScan(
+            Arrays.asList("test"),
+            Arrays.asList("a", "b", "c"),
+            Arrays.asList(R.I64, R.I16, R.I32));


Using the builder to setup the input as it's more readable (IMO)

vbarua · 2024-02-16T21:56:34Z

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

+                    ConsistentPartitionWindow.WindowRelFunctionInvocation.builder()
+                        .declaration(windowFunctionDeclaration)
+                        // lead(a)
+                        .arguments(Arrays.asList(b.fieldReference(input, 0)))


Specify at least one argument to make sure argument conversions are wired in.

vbarua · 2024-02-16T21:57:35Z

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

+
+    io.substrait.proto.Rel protoRel = relProtoConverter.toProto(rel1);
+    io.substrait.relation.Rel rel2 = protoRelConverter.from(protoRel);
+    assertEquals(rel1, rel2);


Use the relProtoConverter and protoRelConverter defined in TestBase to avoid having to set them up ourselves.

vbarua

Changes look good.

Made a small refactor in 05476ed and simplified the test in e595bf9, see my PR comments for my reasoning.

…itionWindowRel (substrait-io#231)

feat: add POJOs for ConsistentPartitionWindow and WindowRelFunctionIn…

aea53cf

…vocation

vbarua reviewed Feb 15, 2024

View reviewed changes

vbarua requested changes Feb 15, 2024

View reviewed changes

fix: address code review comments

1bb9836

bvolpato requested a review from vbarua February 15, 2024 20:38

vbarua added 2 commits February 16, 2024 13:46

refactor: use from builder util to avoid having to manually set all f…

05476ed

…ields

refactor: simplify testing of consistent partition window rel roundtr…

e595bf9

…ipping

vbarua reviewed Feb 16, 2024

View reviewed changes

vbarua approved these changes Feb 16, 2024

View reviewed changes

vbarua merged commit f148bbb into substrait-io:main Feb 16, 2024
8 checks passed

bvolpato mentioned this pull request Feb 23, 2024

feat(isthmus): add WindowRelFunctionConverter #234

Merged

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

feat(pojo): add POJO representation and converters for ConsistentPart…

8f37ee0

…itionWindowRel (substrait-io#231)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add POJOs for ConsistentPartitionWindowRel and WindowRelFunction #231

feat: add POJOs for ConsistentPartitionWindowRel and WindowRelFunction #231

bvolpato commented Feb 14, 2024

vbarua Feb 15, 2024

bvolpato Feb 15, 2024

vbarua left a comment

vbarua Feb 15, 2024

bvolpato Feb 15, 2024

vbarua Feb 15, 2024

bvolpato Feb 15, 2024

vbarua Feb 15, 2024

bvolpato Feb 15, 2024

vbarua Feb 15, 2024

bvolpato Feb 15, 2024

bvolpato commented Feb 15, 2024

vbarua Feb 16, 2024

vbarua Feb 16, 2024

vbarua Feb 16, 2024

vbarua Feb 16, 2024

vbarua Feb 16, 2024

vbarua Feb 16, 2024

vbarua left a comment

feat: add POJOs for ConsistentPartitionWindowRel and WindowRelFunction #231

feat: add POJOs for ConsistentPartitionWindowRel and WindowRelFunction #231

Conversation

bvolpato commented Feb 14, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvolpato commented Feb 15, 2024

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

vbarua left a comment

Choose a reason for hiding this comment