fix(storage): cursor requests are [start, stop] instead of [start, stop)

[jsternberg]

The cursors were previously [start, stop) to be consistent with how flux
requests data, but the underlying storage file store was [start, stop]
because that's how influxql read data. This reverts back the cursor
behavior so that it is now [start, stop] everywhere and the conversion
from [start, stop) to [start, stop] is performed when doing the cursor
request to get the next cursor.

[lesam]

Is it possible to show this bug in a new flux end-to-end test? I see a lot of stuff testing mocks and lower level things, but for this sort of off by one error it would be nice to verify everything is really working end-to-end

[lesam]

I've tried a couple end-to-end tests and I can't make anything fail on master but pass on this branch - looks like range is setting the correct time bounds both on master and in this branch.

Reading the PR again, maybe this is expected? We seem to be changing both ends of the API simultaneously. Is this PR just to make the storage API agree with the docs, without user-facing change?

[lesam]

See comments - I'd like to see either an end-to-end test showing the broken case (so we can make sure it doesn't regress in the future, and confirm it is really fixed end-to-end), or get confirmation that this is a pure refactor without user-visible changes.

[jsternberg]

I'm not sure what effects which storage engine so some of this may just be cleanup. I expect most queries are unaffected, but we did find one specific scenario.

When last() is used as a raw aggregate, a descending cursor is used. The descending cursor pathway opens a key cursor that reads the stop time, which is the start of a descending read, as an inclusive time. This point then gets filtered out by another portion of the code that determines it is not supposed to be present. In some rare circumstances, this one timestamp will be the only one in a TSM file entry. The file store returns that one point, the cursor filters it out, and then the cursor decides the query has no points because no points were returned.

In order to fix this, we had to make all of the APIs choose a consistent method of accessing the file store. Because of influxql setting a precedent, we decided storage APIs have to be [start, stop] or inclusive on both ends. This meant we had to change the flux cursor reading code to adjust its method, which is [start, stop), to [start, stop]. We did this in the place where the flux readers request new cursors to be created from the storage engine. This then meant we had to fix the storage engine code underneath that to consistently use [start, stop]. Previously, they were using a combination of the two. But, in most circumstances, the actual errant point would get filtered out by a higher level. For ascending cursors, the file store would return one too many points, but the cursors would then remove that errant point when it went to merge points.

The unit tests specifically test the underlying behavior for how files can get into bad situations. Generally, the current code works fine after a full compaction happens because the errors pop up when the files are more scattered. Also, these off-by-one errors don't appear to affect the cache.

[lesam]

LGTM