Support negative timestamps for the query engine #7212
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Need to fix a bunch of tests, but this seems to work in local testing. |
jsternberg
force-pushed
the
js-query-engine-negative-timestamps
branch
3 times, most recently
from
d4fb158 to
374f424
Compare
| @@ -3630,7 +3630,7 @@ func TimeRange(expr Expr) (min, max time.Time, err error) { | |||
| } | |||
|
|
|||
| // TimeRangeAsEpochNano returns the minimum and maximum times, as epoch nano, specified by | |||
| // and expression. If there is no lower bound, the start of the epoch is returned | |||
| // and expression. If there is no lower bound, the minimum time is returned | |||
There was a problem hiding this comment.
You updated the comment here, but line 3642 still sets the min to time.Unix(0,0) when the min time is unset.
There was a problem hiding this comment.
Good catch. I went through a bunch of different revisions while doing this and I might have lost some things like this.
jsternberg
force-pushed
the
js-query-engine-negative-timestamps
branch
from
374f424 to
66c00f2
Compare
| // ZeroTime is the Unix nanosecond timestamp for time.Time{}. | ||
| const ZeroTime = int64(-6795364578871345152) | ||
| // ZeroTime is the Unix nanosecond timestamp for no time. | ||
| const ZeroTime = models.MinNanoTime - 2 |
There was a problem hiding this comment.
Please add a comment for the arbitrary subtraction of two.
jsternberg
force-pushed
the
js-query-engine-negative-timestamps
branch
2 times, most recently
from
a921d0a to
82d5a32
Compare
|
@joelegasse I tried my best to add documentation for why the arbitrary values for minimum and maximum times. |
Negative timestamps are now supported. We also now refuse two nanoseconds that are at the edge of the minimum time window. One of the nanoseconds we do not accept is because we need MinInt64 to be used for some internal comparisons in the TSM engine and it was causing an underflow when we subtracted one from the minimum time. The second is so we can have one minimum time that signifies the default minimum that nobody can write to (so we can implicitly rewrite the timestamp on aggregate queries) but still use the explicit timestamp if it is given to us by the user. We aren't able to tell the difference between if the user provided it or if it was implicit without those values being different. If the default minimum time is used with an aggregate query, we rewrite the time to be the epoch for backwards compatibility since we believe that's more important than supporting that extra nanosecond.
jsternberg
force-pushed
the
js-query-engine-negative-timestamps
branch
from
82d5a32 to
10029ca
Compare
|
Thanks for adding the comments. LGTM |
|
👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #7194.