fix(isthmus) : make arguments of rollup function nullable in substrait proto->Rel conversion

[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.