fix(isthmus): improve derivation of AggregateRel return type

[ZorinAnton]

Minimal reproducing query:

select charcol from (
  select charcol, count(*) from (values('a')) as t(charcol) group by rollup(charcol)
)

If this query gets converted to subtrait proto and then to substrait pojo, the latter looks as follows:

Plan{version=Version{major=0, minor=74, patch=0, producer=isthmus}, roots=[Root{input=Project{remap=Remap{indices=[2]}, input=Aggregate{input=VirtualTableScan{initialSchema=NamedStruct{struct=Struct{nullable=false, fields=[FixedChar{nullable=false, length=1}]}, names=[CHARCOL]}, rows=[StructLiteral{nullable=false, fields=[FixedCharLiteral{nullable=false, value=a}]}]}, groupings=[Grouping{expressions=[FieldReference{segments=[StructField{offset=0}], type=FixedChar{nullable=false, length=1}}]}, Grouping{expressions=[]}], measures=[Measure{function=AggregateFunctionInvocation{declaration=count:, arguments=[], options=[], aggregationPhase=INITIAL_TO_RESULT, sort=[], outputType=I64{nullable=false}, invocation=ALL}}]}, expressions=[FieldReference{segments=[StructField{offset=0}], type=FixedChar{nullable=false, length=1}}]}, names=[CHARCOL]}], expectedTypeUrls=[]}

The ROLLUP function makes grouping columns nullable. This was causing a Calcite validation error because the outer projection's CHARCOL column was incorrectly defined as non-nullable. This fix corrects the column type, allowing the conversion to succeed.

Plan{version=Version{major=0, minor=74, patch=0, producer=isthmus}, roots=[Root{input=Project{remap=Remap{indices=[2]}, input=Aggregate{input=VirtualTableScan{initialSchema=NamedStruct{struct=Struct{nullable=false, fields=[FixedChar{nullable=false, length=1}]}, names=[CHARCOL]}, rows=[StructLiteral{nullable=false, fields=[FixedCharLiteral{nullable=false, value=a}]}]}, groupings=[Grouping{expressions=[FieldReference{segments=[StructField{offset=0}], type=FixedChar{nullable=false, length=1}}]}, Grouping{expressions=[]}], measures=[Measure{function=AggregateFunctionInvocation{declaration=count:, arguments=[], options=[], aggregationPhase=INITIAL_TO_RESULT, sort=[], outputType=I64{nullable=false}, invocation=ALL}}]}, expressions=[FieldReference{segments=[StructField{offset=0}], type=FixedChar{nullable=true, length=1}}]}, names=[CHARCOL]}], expectedTypeUrls=[]}

[vbarua]

Just saw your new update from this morning. So this issue is associated with the presence of multiple grouping set? It would be helpful to have a direct/minimal reproducer for this to be able to understand what exactly the issue is. The TPCDS test hits the issue, but it doesn't really help identify it.

We should also update the title indicate the issue that's being fixed. Fixing TCPDs 67 is a side-effect of fixing an actual underlying issue.

[ZorinAnton]

I've updated the PR title and commit message from "fix tpcds query 67" to be more descriptive.

I've added the new test case to the Substrait2SqlTest class. I wasn't entirely sure this was the right spot, as the class name is a bit confusing (it seems to cover a full sql<->substrait roundtrip). However, since it already contains tests for various SQL clauses, I thought adding the rollup test here would be consistent.

Please let me know if you think there's a better place for this test.

[vbarua]

Thanks for the reproducer for this. I'm digging into this a bit because there's a general pattern of bugs we've also noticed due to disagreement between Calcite and Substrait as to nullabilities.

[vbarua]

Haven't found time to dig into this, but I figured I would update on why I haven't merged. Effectively, tweaking the return type like you have isn't really in keeping with the spec for AggregateRel, which as defined now should output all inputs types as given without tweaking nullabilities. This isn't probably quite correct, and the issue arises when Calcite and Substrait have different expectations around the nullabilities of the columns. Calcite is probably correct in this case, but I think it's worth understanding exactly why it's wrong to update the core spec and make our AggregateRel more correct.

Variants of this have arisen before #336, and there is actually an issue to look into this further #379.

[ZorinAnton]

AggregateRel, which as defined now should output all inputs types as given without tweaking nullabilities.

I couldn't find that statement in the spec. However, the logic applied in the PR was a bit different than in the spec, that says

The values for the grouping expression columns that are not part of the grouping set for a particular record will be set to null.

Instead, all grouping columns were made nullable in case of more than one grouping sets. I have fixed the logic according to the spec. This change doesn't affect the ROLLUP function (the target of this PR) because ROLLUP generates grouping sets in a way that no single grouping column appears in all of them.

[vbarua]

Oh, you're right we do actually have something in the spec for grouping sets:

The columns for grouping expressions that do not appear in all grouping sets will be nullable (regardless of the nullability of the type returned by the grouping expression) to accomodate the null insertion.

and

To further disambiguate which record belongs to which grouping set, an aggregate relation with more than one grouping set receives an extra i32 column on the right-hand side. The value of this field will be the zero-based index of the grouping set that yielded the record.

[ZorinAnton]

I tried adding an extra integer, while it works with minimal reproducer, it somehow breaks the calcite validation when partition by is used together with rollup. I would suggest to address this issue in a separate PR.

[nielspardon]

I tried adding an extra integer, while it works with minimal reproducer, it somehow breaks the calcite validation when partition by is used together with rollup. I would suggest to address this issue in a separate PR.

@vbarua are you ok with merging this fix for the nullability and tackle the missing grouping set index in a separate PR? I created this issue for adding the grouping set index: #527

[vbarua]

I'm not entirely sure this will work as expected because I'm not sure if expression equality / hash coding is well defined. That being said, we're already working on switching to the new grouping field mechanism that will make this obsolete in #521

[vbarua]

Apologies for the delay. I agree that this is reasonable to merge in as its strictly improving the existing deficiency, even if it's not fully solving it. The followup issue tracks the remaining work #527

I'll merge this myself later today, I want to make include a succint description in the body for the release notes. Strictly speaking, while this fixes rollup queries, that's more a side-effect of correcting the type-derivation.

[vbarua]

Ah, a test appears to have broken following the merge of #518 😢

I don't have time to follow up and dig into it, but if either of @ZorinAnton or @nielspardon can address it, I can prioritize merging this PR in this week.

[ZorinAnton]

I can prioritize merging this PR in this week.

Thank you, it would be great. After merging the failure has gone.

[vbarua]

Thanks for your work on this @ZorinAnton