search

substrait-io / substrait-java

Apache License 2.0
72 stars 70 forks source link

fix(isthmus): allow for conversion of plans containing Calcite aggregate functions #230

Closed bvolpato closed 4 months ago

bvolpato commented 4 months ago

Some of the Calcite CoreRules might bring their own functions at a late stage, so I've decided to make the converter flexible enough and handle the superclass types correctly (since we already do a similar conversion to APPROX_COUNT_DISTINCT in the same spot).

Another alternative would be to improve the matchers and check if names match or if it's an instanceof the passing call -- today signatures is an IdentityHashMap, so it's really strict.

bvolpato commented 4 months ago

FYI Check failures are unrelated to the change:

/home/runner/work/substrait-java/substrait-java/core/src/test/java/io/substrait/type/proto/LocalFilesRoundtripTest.java:61: error: switch expressions are not supported in -source 8
    return switch (pathTypeCase) {
           ^
  (use -source 14 or higher to enable switch expressions)
/home/runner/work/substrait-java/substrait-java/core/src/test/java/io/substrait/type/proto/LocalFilesRoundtripTest.java:62: error: switch rules are not supported in -source 8
      case URI_PATH -> builder.pathType(FileOrFiles.PathType.URI_PATH).path("path");
bvolpato commented 4 months ago

Thanks for these changes!

I was poking around to add the conversionHandlesBuiltInSum0CallAddedByRule test and made some small changes along the way as well.

If the code in 6c0c6fd looks good to you, we can merge this in.

It looks good to me! Thanks for all the improvements / suggestions!