search

substrait-io / substrait-java

Apache License 2.0
72 stars 70 forks source link

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

Closed vbarua closed 5 months ago

vbarua commented 6 months ago

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 commented 6 months ago

Fixes https://github.com/substrait-io/substrait-java/issues/155

vbarua commented 6 months ago

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 commented 6 months ago

Addresses https://github.com/substrait-io/substrait-java/issues/155

vibhatha commented 6 months ago

@vbarua seems like there is a conflict.

vbarua commented 5 months ago

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.