fix(storage): Detect need for descending cursor in WindowAggregate #21140
Conversation
query/stdlib/universe/last_test.flux
Outdated
| import "planner" | ||
| import "csv" | ||
|
|
||
| testcase last_bug{ |
There was a problem hiding this comment.
Format nitpick. Separate the bracket from the text.
| @@ -23,6 +23,23 @@ type windowAggregateResultSet struct { | |||
| err error | |||
| } | |||
|
|
|||
| func IsAscendingWindowAggregate(req *datatypes.ReadWindowAggregateRequest) bool { | |||
| } | ||
|
|
||
| if req.Aggregate[0].Type == datatypes.AggregateTypeLast { | ||
| if req.Window == nil { |
There was a problem hiding this comment.
Can you add a comment for this section while you're here explaining the checks in this code? It wasn't here previously so no requirement but it would be nice to comment this.
v1/services/storage/store.go
Outdated
| @@ -71,7 +71,8 @@ func (s *Store) WindowAggregate(ctx context.Context, req *datatypes.ReadWindowAg | |||
| return nil, err | |||
| } | |||
|
|
|||
| shardIDs, err := s.findShardIDs(database, rp, false, start, end) | |||
| ascending := reads.IsAscendingWindowAggregate(req) | |||
There was a problem hiding this comment.
Comment here too since it's non-obvious what's going on here.
|
@danxmoran It should address #20486 and #20722. I am not sure if it will address #20595. I will try to confirm and get back to you. |
|
@danxmoran I believe those are all the same bug. |
|
I believe I follow the logic ok. I pushed some cosmetic suggestions into this branch |
query/stdlib/universe/last_test.flux
Outdated
| import "planner" | ||
| import "csv" | ||
|
|
||
| testcase last_bug { |
There was a problem hiding this comment.
Sorry to comment on this again but can you rename this to last_multi_shard.
storage/reads/aggregate_resultset.go
Outdated
| if nAggs := len(req.Aggregate); nAggs != 1 { | ||
| return nil, errors.Errorf(errors.InternalError, "attempt to create a windowAggregateResultSet with %v aggregate functions", nAggs) | ||
| // IsAscendingWindowAggregate checks two things: If the request passed in | ||
| // is using the `last` aggregate type, and if it has a window. If both |
There was a problem hiding this comment.
If it doesn't have a window.
Co-authored-by: Phil Bracikowski <pbracikowski+git@influxdata.com>
scbrickley
force-pushed
the
last-planner-bug
branch
from
70cc000
to
5279639
Compare
influxdata/flux#3480
Fixes case where the storage engine was reading through shards in ascending order when it needed to read in descending order.