Skip to content

Commit ff55294

Browse files
authored
Serialize missing field createDefaultIfEmpty into streaming aggregate plan continuations (#3211)
There is missing state that was introduced to the `AggregateCursor` and its accompanying plan in #3092. However, this state was not added to the continuation, which results in unexpected behavior when we try to deserialize the plan's continuation (effectively, it reverts to the old behavior of the plan operator). We were already correctly _de_serializing this information, so the upshot is that any plan that is started after this current version should exhibit the correct behavior, but we get incorrect behavior when a plan is started on an older version. This fixes #3096. I was able to get test coverage of this part of the code by enabling `FORCE_CONTINUATIONS` mode on `aggregateIndexTests`.
1 parent 49bfe50 commit ff55294

File tree

3 files changed

+62
-3
lines changed

3 files changed

+62
-3
lines changed

fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryStreamingAggregationPlan.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,8 @@ public PRecordQueryStreamingAggregationPlan toProto(@Nonnull final PlanSerializa
370370
}
371371
builder.setGroupingKeyAlias(groupingKeyAlias.getId())
372372
.setAggregateAlias(aggregateAlias.getId())
373-
.setCompleteResultValue(completeResultValue.toValueProto(serializationContext));
373+
.setCompleteResultValue(completeResultValue.toValueProto(serializationContext))
374+
.setIsCreateDefaultOnEmpty(isCreateDefaultOnEmpty);
374375
return builder.build();
375376
}
376377

yaml-tests/src/test/java/YamlIntegrationTests.java

-2
Original file line numberDiff line numberDiff line change
@@ -129,8 +129,6 @@ public void createDropCreateTemplate(YamlTest.Runner runner) throws Exception {
129129
}
130130

131131
@TestTemplate
132-
@ExcludeYamlTestConfig(value = YamlTestConfigFilters.DO_NOT_FORCE_CONTINUATIONS,
133-
reason = "Continuation verification (https://github.com/FoundationDB/fdb-record-layer/issues/3096)")
134132
public void aggregateIndexTests(YamlTest.Runner runner) throws Exception {
135133
runner.runYamsql("aggregate-index-tests.yamsql");
136134
}

yaml-tests/src/test/resources/aggregate-index-tests.yamsql

+60
Original file line numberDiff line numberDiff line change
@@ -132,8 +132,20 @@ test_block:
132132
# At some point, should be able to roll up values from the aggregate index. However, even
133133
# controlling for that, it can still use the index
134134
- query: select max(col2) from T1 use index (mv8);
135+
- supported_version: !current_version
135136
- explain: "ISCAN(MV8 <,>) | MAP (_ AS _0) | AGG (max_l(_._0.COL2) AS _0) | ON EMPTY NULL | MAP (_._0._0 AS _0)"
136137
- result: [{!l 13}]
138+
-
139+
- query: select max(col2) from T1 use index (mv8);
140+
- maxRows: 1
141+
# Cannot use FORCE_CONTINUATIONS with older versions due to: https://github.com/FoundationDB/fdb-record-layer/issues/3096
142+
# (Extra values being produced after exhausting source of an aggregate cursor)
143+
# Can remove once we do not care about backwards compatibility before !current_version
144+
- initialVersionLessThan: !current_version
145+
# Different (incorrect) behavior for different past versions
146+
- initialVersionAtLeast: !current_version
147+
- result: [{!l 13}]
148+
- result: []
137149
-
138150
# Min/max indexes need keep what amounts to a standard value index on their keys (in order to properly look up
139151
# the min/max). That index should be usable for normal queries just like a value index. Note that the scan is
@@ -147,14 +159,43 @@ test_block:
147159
- result: [{!l 5}, {!l 4}, {!l 3}, {!l 2}, {!l 1}]
148160
-
149161
- query: select min(col3) from T2 group by col1, col2;
162+
- supported_version: !current_version
150163
- explain: "ISCAN(MV2 <,>) | MAP (_ AS _0) | AGG (min_l(_._0.COL3) AS _0) GROUP BY (_._0.COL1 AS _0, _._0.COL2 AS _1) | MAP (_._1._0 AS _0)"
151164
- result: [{!l 1}, {!l 2}, {!l 3}]
165+
-
166+
- query: select min(col3) from T2 group by col1, col2;
167+
# Cannot use FORCE_CONTINUATIONS with older versions due to: https://github.com/FoundationDB/fdb-record-layer/issues/3096
168+
# (Extra values being produced after exhausting source of an aggregate cursor)
169+
# Can remove once we do not care about backwards compatibility before !current_version
170+
- maxRows: 1
171+
- initialVersionLessThan: !current_version
172+
# Different (incorrect) behavior for different past versions
173+
- initialVersionAtLeast: !current_version
174+
- result: [{!l 1}]
175+
- result: [{!l 2}]
176+
- result: [{!l 3}]
177+
- result: []
152178
-
153179
# this should use the aggregate index in the future, for now, it is using streaming aggregate
154180
# over base table scan.
155181
- query: select max(col2) from t2;
182+
- supported_version: !current_version
156183
- explain: "ISCAN(MV3 <,>) | MAP (_ AS _0) | AGG (max_l(_._0.COL2) AS _0) | ON EMPTY NULL | MAP (_._0._0 AS _0)"
157184
- result: [{!l 2}]
185+
-
186+
- query: select max(col2) from t2;
187+
# Cannot use FORCE_CONTINUATIONS with older versions due to: https://github.com/FoundationDB/fdb-record-layer/issues/3096
188+
# (Extra values being produced after exhausting source of an aggregate cursor)
189+
# Can remove once we do not care about backwards compatibility before !current_version
190+
- supported_version: !current_version
191+
- maxRows: 1
192+
- initialVersionLessThan: !current_version
193+
- result: [{!l 2}]
194+
- result: [{!null _}]
195+
- result: [{!l 2}] # ad infinitum
196+
- initialVersionAtLeast: !current_version
197+
- result: [{!l 2}]
198+
- result: []
158199
-
159200
- query: select col1, sum(col2) from T1 USE INDEX (vi1) group by col1;
160201
- explain: "ISCAN(VI1 <,>) | MAP (_ AS _0) | AGG (sum_l(_._0.COL2) AS _0) GROUP BY (_._0.COL1 AS _0) | MAP (_._0._0 AS COL1, _._1._0 AS _1)"
@@ -208,12 +249,28 @@ test_block:
208249
-
209250
# Permuted max index can also be used to evaluate other aggregate functions via aggregation and roll-up
210251
- query: select col3, sum(col2) as s from t2 use index (mv9) where col1 = 1 group by col1, col3 order by col3 asc;
252+
- supported_version: !current_version
211253
- explain: "ISCAN(MV9 [EQUALS promote(@c20 AS LONG)]) | MAP (_ AS _0) | AGG (sum_l(_._0.COL2) AS _0) GROUP BY (_._0.COL1 AS _0, _._0.COL3 AS _1) | MAP (_._0._1 AS COL3, _._1._0 AS S)"
212254
- result: [{COL3: 1, S: 1}, {COL3: 2, S: 2}, {COL3: 100, S: 1}, {COL3: 200, S: 2}]
255+
-
256+
- query: select col3, sum(col2) as s from t2 use index (mv9) where col1 = 1 group by col1, col3 order by col3 asc;
257+
# Cannot use FORCE_CONTINUATIONS with older versions due to: https://github.com/FoundationDB/fdb-record-layer/issues/3096
258+
# (Extra values being produced after exhausting source of an aggregate cursor)
259+
# Can remove once we do not care about backwards compatibility before !current_version
260+
- maxRows: 0
261+
- result: [{COL3: 1, S: 1}, {COL3: 2, S: 2}, {COL3: 100, S: 1}, {COL3: 200, S: 2}]
213262
-
214263
- query: select col3, sum(col2) as s from t2 use index (mv9) where col1 = 1 group by col1, col3 order by col3 desc;
264+
- supported_version: !current_version
215265
- explain: "ISCAN(MV9 [EQUALS promote(@c20 AS LONG)] REVERSE) | MAP (_ AS _0) | AGG (sum_l(_._0.COL2) AS _0) GROUP BY (_._0.COL1 AS _0, _._0.COL3 AS _1) | MAP (_._0._1 AS COL3, _._1._0 AS S)"
216266
- result: [{COL3: 200, S: 2}, {COL3: 100, S: 1}, {COL3: 2, S: 2}, {COL3: 1, S: 1}]
267+
-
268+
- query: select col3, sum(col2) as s from t2 use index (mv9) where col1 = 1 group by col1, col3 order by col3 desc;
269+
# Cannot use FORCE_CONTINUATIONS with older versions due to: https://github.com/FoundationDB/fdb-record-layer/issues/3096
270+
# (Extra values being produced after exhausting source of an aggregate cursor)
271+
# Can remove once we do not care about backwards compatibility before !current_version
272+
- maxRows: 0
273+
- result: [{COL3: 200, S: 2}, {COL3: 100, S: 1}, {COL3: 2, S: 2}, {COL3: 1, S: 1}]
217274
# -
218275
# # grouping by constant is not yet supported.
219276
# - query: select sum(col2) from t1 group by 3,2,1;
@@ -230,6 +287,9 @@ test_block:
230287
- result: [{!l 100}, {!l 200}, {!l 400}]
231288
-
232289
- query: select min_ever(col3) from t2
290+
# Cannot enable FORCE_CONTINUATIONS with ungrouped aggregate scan because of: https://github.com/FoundationDB/fdb-record-layer/issues/3206
291+
- maxRows: 0
292+
- explain: "AISCAN(MV7 <,> BY_GROUP -> [_0: VALUE:[0]]) | MAP (_ AS _0) | ON EMPTY NULL | MAP (_._0._0 AS _0)"
233293
- result: [{!l 1}]
234294
-
235295
- query: select min_ever(col3) from t2

0 commit comments

Comments
 (0)