[CALCITE-6261] AssertionError with field pruning & duplicate agg calls

[nielspardon]

This PR fixes issues [CALCITE-6261] and [CALCITE-5888] which experienced an AssertionError when using field pruning with duplicate aggregation calls since Apache Calcite v1.35.0.

The issue was introduced with the code change from [CALCITE-4334] where the groupSet and groupSets variables were replaced by groupSet2 and groupSets2. The replacement was not done consistently so that the code for the elimination of duplicate aggregation calls was still using the groupSet and groupSets variables instead of the new variables.

The issue was caused by groupSet no longer containing the permuted field indices after the pruning of unnecessary fields and still containing the old field indices while the permuted field indices are now only available in groupSet2.

PR #3700 also attempted to fix this in parallel to my work on this PR due to some misunderstanding about the status of [CALCITE-6261] but did not consistently make use of the new variable groupSet2 in the agg call deduplication code.

Also, this PR contains unit tests for reproducing both scenarios from [CALCITE-6261] and [CALCITE-5888].

During testing I also tried reproducing the issue using SQL and the Quidem test suite but was unable to reproduce it via SQL.

[mihaibudiu]

when is groupSet.cardinality() != groupSet2.cardinality()?

[nielspardon]

From my understanding the return value of groupSet.cardinality() should always be the same as for groupSet2.cardinality() in the current implementation. My main point is that with all the logic at the beginning of the aggregate_() method which introduces groupSet2 we should use it consistently in the latter part of the method.

[mihaibudiu]

Sure, but this means that #3700 is actually equivalent to this fix.
So that PR probably fixes both issues as well.
Is there a more descriptive name that could be used instead of groupSet2? Maybe that's the root problem: it is very hard to tell what groupSet2 means. But if it was called e.g., replacementGroupSet, then it would become apparent where it is misused.

[nielspardon]

Sure, but this means that #3700 is actually equivalent to this fix.
So that PR probably fixes both issues as well.

Yes, functionally the other PR does fix the AssertionError and the functional outcome is the same.

Is there a more descriptive name that could be used instead of groupSet2? Maybe that's the root problem: it is very hard to tell what groupSet2 means. But if it was called e.g., replacementGroupSet, then it would become apparent where it is misused.

Yes, I also think the problem was introduced because of the ambiguity of the variables in the first place. A first thought I had was that maybe some refactoring of this method into smaller methods like extracting the agg call deduplication code into its own method could help to have a clearer separation with less ambiguity.

[mihaibudiu]

I have approved the PR, but if you are willing to improve the code I will wait for a new commit.

[nielspardon]

I can give it a shot on Monday when I'm back in the office.

[nielspardon]

• I renamed groupSet2 and groupSets2 to groupSetAfterPruning and groupSetsAfterPruning respectively to give them more obvious names.
• I also renamed the helper variable map to sourceFieldToTargetFieldMap to make it more obvious.
• I moved the field pruning and agg call deduplication code into its own pruneAggregateInputFieldsAndDeduplicateAggCalls method to reduce the length of the original aggregate_() method.

[sonarcloud]

Quality Gate passed

Issues
4 New issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[rubenada]

LGTM