Infinite continuations: sum(*) returns alternating rows forever

[ohadzeliger]

Adding maxRows:1 to the yaml test keeps returning two rows forever (the result row and an empty row), the continuation is never atEnd().

Steps To Reproduce:

1. For example, in the test aggregate-index-tests.yamsql, modify the test to be:

2.    - query: select max(col2) from T1 use index (mv8);
   - maxRows: 1
   - explain: "ISCAN(MV8 <,>) | MAP (_ AS _0) | AGG (max_l(_._0.COL2) AS _0) | ON EMPTY NULL | MAP (_._0._0 AS _0)"
   - result: [{!l 13}]
   - result: [{!null _}]
   - result: [{!l 13}]
   - result: [{!null _}]
   - result: [{!l 13}]
   - result: [{!null _}]
   - result: [{!l 13}]
   - result: [{!null _}]
   

3. And the test will always fail with “Query returned more results, but no more were expected after line …”

Note that this behaves differently when running in mixed mode, where the error is:

Caused by: com.google.common.base.VerifyException
at com.google.common.base.Verify.verify(Verify.java:102)
at com.apple.foundationdb.relational.recordlayer.query.QueryPlan$ContinuedPhysicalQueryPlan.validatePlanAgainstEnvironment(QueryPlan.java:520)
at com.apple.foundationdb.relational.recordlayer.query.QueryPlan$PhysicalQueryPlan.executePhysicalPlan(QueryPlan.java:361)
at com.apple.foundationdb.relational.recordlayer.query.QueryPlan$PhysicalQueryPlan.executeInternal(QueryPlan.java:252)
at com.apple.foundationdb.relational.recordlayer.query.QueryPlan$PhysicalQueryPlan.executeInternal(QueryPlan.java:112)

[alecgrieser]

We now know what the difference between mixed-mode and non-mixed results here is about (it's a testing artifact based on how the framework handles receiving null/starting continuations--it's also been addressed by #3186). The continuation problem has to do with how the aggregate cursor handles continuations.

Namely, this is a consequence of the fact that the AggregateCursor does not remember whether it has already returned a value, so when maxRows is 1, it effectively goes:

1. First iteration: start from the beginning, return value and continuation pointing to the last record in the scan
2. Second iteration: pick up from where the previous continuation left off, scanning zero values, so return the default null value with a START continuation (as it always does when it returns the default value)
3. Third iteration: now we have a start continuation, so go back to step 1

Then this repeats ad infinitum. This is something @pengpeng-lu noted in #3177 (for #3180). It's also notable that I think this should go away when #3107 can be done in 4.2, as this doesn't happen when isCreateDefaultOnEmpty is set to false.

[alecgrieser]

Also notable: this happens for all aggregates, not just sum

[alecgrieser]

Actually, my comment about:

It's also notable that I think this should go away when #3107 can be done in 4.2, as this doesn't happen when isCreateDefaultOnEmpty is set to false.

Led me to another discover: isCreateDefaultOnEmpty is a field that is already being set by default on the AggregateCursor on 4.1, but we're still seeing the error. As it turns out, we forgot to serialize the isCreateDefaultOnEmpty configuration, so when the value goes back to the older value, it then defaults to the older behavior. If we serialize that, then we can at least have the continuations be correct going backwards.