feat(isthmus): improved Calcite support for Substrait Aggregate rels

[vbarua]

Substrait Aggregates that contain expressions that are not field
references and/or grouping keys that are not in input order require
extra processing to be converted to Calcite Aggregates successfully AND
correctly

[vbarua]

Fixes #155

[vbarua]

Added arithmetic aggregate functions that take expression directly.

[vbarua]

I needed to nest some functions within aggregate function calls for testing purposes, so I added some easy ones.

[vbarua]

Filed an issues for this: #215

[patientstreetlight]

Mostly had minor style questions/comments, but overall I think this looks good, and very welcome.

[vbarua]

To briefly summarise what this PR is about. Calcite has a more restrictive definition of aggregate relations than Substrait does, which means that not all Substrait aggregates can be converted to Calcite aggregates. The primary restriction is that Calcite aggregates can only take field references in various places that Substrait allows full expressions.

Luckily, this is easy to address by simply moving those expressions to a project before the aggregate, and then updating the aggregate to use references.

The various comments in the PreCalciteAggregateValdidator try and document these various restrictions in more detail.

[vbarua]

Addresses #155

[vibhatha]

@vbarua I am not very fluent with the Calcite component, so I will leave someone else to verify this part.

[vibhatha]

General comment: I see the expected output is hand-calculated (meaning determined by ourselves), is there a way to use Calcite to get the optimal and validate with it? This is an out of scope area for me, but just curious if we can do things differently.

[vbarua]

The transformation code that I'm testing here is purely going from Substrait -> Substrait. My intent with these tests was to demonstrate that the transformToValidCalciteAggregate(aggregate) produces equivalent Substrait plans, no Calcite required.

Calcite only becomes relevant in the validation code when we call

    // Substrait POJO -> Calcite
    new SubstraitToCalcite(EXTENSION_COLLECTION, typeFactory).convert(pojo);

which tries to convert the given input to a Calcite plan. One thing that's hard to show here is that if you comment out

    if (!PreCalciteAggregateValidator.isValidCalciteAggregate(aggregate)) {
      aggregate =
          PreCalciteAggregateValidator.PreCalciteAggregateTransformer
              .transformToValidCalciteAggregate(aggregate);
    }

in the SubstraitRelNodeConverter#visit(Aggregate aggregate) method all of these fail.

is there a way to use Calcite to get the optimal and validate with it?

Something that's kind of cursed about this is that Calcite already does something like this, but only when you give it SQL input. When it converts from its SqlNode representation to the RelNode representation it applies similar transforms to this.

I don't think copying exactly what Calcite does is a good idea here, because I'd rather perform straightforward (if verbose) transformation here that may not necessarily be the best but is easier to reason about. This also happens before any actual optimizations a user would perform with Calcite, as this transformation is just to allow us to convert the Substrait plan into a Calcite representation.

Does this answer what you were asking?

[vibhatha]

@vbarua seems like there is a conflict.

[vibhatha]

@vbarua Thanks for working on this. The overall change LGTM!
Though I would appreciate someone with better Calcite knowledge take a look.

[vbarua]

I'm going to merge these changes. They will only take effect for substrait plans that would previously have resulted in exceptions, so they shouldn't be surprising anyone.