feat: support reading Substrait plans with Window Functions #165

vbarua · 2023-08-10T22:27:53Z

No description provided.

BREAKING CHANGE: various public functions that took the AggregateFunction.AggregationInvocation proto now take the POJO equivalent Expression.AggregationInvocation.

vbarua · 2023-08-10T22:32:03Z

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

-          bound =
-              Expression.WindowFunction.Bound.newBuilder()
-                  .setUnbounded(unboundedProto)
-                  .setPreceding(preceding)


This was problematic for two reasons:

unbounded and proceeding are both part of the same one of, so both should not be set.

The default offset for preceding is set to 0 here, which is dissalowed per the spec (offset must be a positive integer). I noticed this issue when I added the LiteralToWindowBoundOffset converter which checked for the positivity of the offset.

vbarua · 2023-08-10T22:36:35Z

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

-        var offset = boundedWindowBound.offset();
-        boolean isPreceding = boundedWindowBound.direction() == WindowBound.Direction.PRECEDING;
-        io.substrait.expression.Expression.I32Literal offsetLiteral =
-            (io.substrait.expression.Expression.I32Literal) offset;


The cast of the didn't always succeed. Reading plans in from protos, the offset is encoded as a long which is converted to an I64Literal. Additionally, depending on how Calcite encodes the offset as well, these could potentially be I8 or I16. The possibilities are handled in the LiteralToWindowBoundOffset visitor.

vbarua · 2023-08-10T22:40:43Z

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

+            .partitionBy(partitionExprs)
+            .orderBy(sortFields)
+            .lowerBound(lowerBound)
+            .upperBound(upperBound)
            .build();


The fact that WindowFunctionInvocation and Window both share so many of the same fields seem like a bit of a code smell. I think it's possible to reduce the duplication, but decided to leave that off for a different change.

vbarua · 2023-08-10T23:07:48Z

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

  }

  @Value.Immutable
  abstract static class BoundedWindowBound implements WindowBound {

    @Override
    public BoundedKind boundedKind() {
-      return BoundedKind.UNBOUNDED;
+      return BoundedKind.BOUNDED;
    }


This was an existing bug which caused round-trip failures.

vbarua · 2023-08-10T23:08:21Z

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

@@ -420,7 +421,8 @@ public Expression visit(io.substrait.expression.Expression.Window expr) throws R
        expr.partitionBy().stream()
            .map(e -> e.accept(this))
            .collect(java.util.stream.Collectors.toList());
-    var builder = Expression.WindowFunction.newBuilder();
+    var outputType = expr.getType().accept(typeProtoConverter);


The missing output type caused round-trip failures.

vbarua · 2023-08-10T23:08:48Z

The changes in this PR allows substrait-java to read in Substrait plans containing window functions (the existing machinary only allowed for generating these plans).

Various roundtrip tests are included in WindowFunctionTest. These tests helped my identify some bugs in the existing code to convert POJO window bounds to PROTO window bounds. I've simplified the handling of these as well.

vbarua · 2023-08-10T23:11:17Z

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

+          functionVariant = lookup.getWindowFunction(functionReference, extensions);
+        } catch (RuntimeException e) {
+          // TODO: Ideally we shouldn't need to catch a RuntimeException to be able to attempt our
+          // second lookup


Not as part of this PR, but I think it would be good to move substrait-java and isthmus away from throwing RuntimeExceptions and towards a hierarchy of checked exceptions to improve library economics.

jacques-n · 2023-08-11T18:31:59Z

@jinfengni , can you please review?

vbarua · 2023-08-11T20:59:59Z

Note: this work depends on the refactor in #164

vbarua · 2023-08-14T15:24:27Z

PR to add this behaviour to the spec is substrait-io/substrait#540

EpsilonPrime

Looks good to me.

isthmus/src/test/java/io/substrait/isthmus/WindowFunctionTest.java

…t-io#165) * fix: incorrect BoundedKind for BoundedWindowBound * feat: add mapping for sum0 aggregate function * feat(pojos): convenience methods for working with Window Bounds

vbarua added 2 commits August 9, 2023 13:47

refactor(pojos): avoid using raw proto enums in POJO layer

833e666

BREAKING CHANGE: various public functions that took the AggregateFunction.AggregationInvocation proto now take the POJO equivalent Expression.AggregationInvocation.

fix: incorrect BoundedKind for BoundedWindowBound

ec1a8a4

vbarua commented Aug 10, 2023

View reviewed changes

vbarua force-pushed the vbarua/window-function-support branch from d95c04f to 7d0ed4b Compare August 10, 2023 22:39

vbarua commented Aug 10, 2023

View reviewed changes

vbarua added 3 commits August 10, 2023 15:53

feat: add mapping for sum0 aggregate function

121c3aa

feat(pojos): convenience methods for working with Window Bounds

c93f826

feat: support window function lookup

418ad58

vbarua force-pushed the vbarua/window-function-support branch from 7d0ed4b to 6ed43ef Compare August 10, 2023 22:56

feat: support reading of Substrait plans containing window functions

9c65d6b

vbarua force-pushed the vbarua/window-function-support branch from 6ed43ef to 9c65d6b Compare August 10, 2023 22:58

vbarua marked this pull request as ready for review August 10, 2023 23:00

vbarua requested a review from jacques-n August 10, 2023 23:00

vbarua commented Aug 10, 2023

View reviewed changes

EpsilonPrime approved these changes Aug 14, 2023

View reviewed changes

EpsilonPrime reviewed Aug 14, 2023

View reviewed changes

isthmus/src/test/java/io/substrait/isthmus/WindowFunctionTest.java Show resolved Hide resolved

Base automatically changed from vbarua/pojo-for-aggregation-phase to main August 14, 2023 23:35

refactor: typo

033ff03

vbarua force-pushed the vbarua/window-function-support branch from 35b276b to 033ff03 Compare August 17, 2023 20:16

vbarua merged commit 93c6db5 into main Aug 17, 2023
7 checks passed

vbarua deleted the vbarua/window-function-support branch August 17, 2023 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support reading Substrait plans with Window Functions #165

feat: support reading Substrait plans with Window Functions #165

vbarua commented Aug 10, 2023

vbarua Aug 10, 2023

vbarua Aug 10, 2023

vbarua Aug 10, 2023

vbarua Aug 10, 2023 •

edited

Loading

vbarua Aug 10, 2023

vbarua commented Aug 10, 2023 •

edited

Loading

vbarua Aug 10, 2023

jacques-n commented Aug 11, 2023

vbarua commented Aug 11, 2023

vbarua commented Aug 14, 2023

EpsilonPrime left a comment

feat: support reading Substrait plans with Window Functions #165

feat: support reading Substrait plans with Window Functions #165

Conversation

vbarua commented Aug 10, 2023

vbarua Aug 10, 2023

Choose a reason for hiding this comment

vbarua Aug 10, 2023

Choose a reason for hiding this comment

vbarua Aug 10, 2023

Choose a reason for hiding this comment

vbarua Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

vbarua Aug 10, 2023

Choose a reason for hiding this comment

vbarua commented Aug 10, 2023 • edited Loading

vbarua Aug 10, 2023

Choose a reason for hiding this comment

jacques-n commented Aug 11, 2023

vbarua commented Aug 11, 2023

vbarua commented Aug 14, 2023

EpsilonPrime left a comment

Choose a reason for hiding this comment

vbarua Aug 10, 2023 •

edited

Loading

vbarua commented Aug 10, 2023 •

edited

Loading