Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timeseries starting from 0 are unsupported #37

Open
karanlyons opened this issue Jun 7, 2018 · 1 comment
Open

Timeseries starting from 0 are unsupported #37

karanlyons opened this issue Jun 7, 2018 · 1 comment

Comments

@karanlyons
Copy link
Contributor

> ts.create test 100
> ts.add test 0 5
> ts.range test 0 100
(empty list or set)
> ts.add test 1 5
> ts.range test 0 100
1) 1) (integer) 1
   2) "5"

This is likely due to a comparison check between timestamp and lastTimestamp. This isn’t actually a personal issue for me, but it’s irksome and I’d like to submit a pull for it if there’s not a reasonable reason for this behavior.

Note that in addition it’s perhaps wise not to consider the timestamp as any specific unit to begin with as I don’t think such an assumption is made anywhere in code (beyond just variable names) and truly our only constraint is that the value for timestamp be monotonic and >= 0 (though currently with this bug we require >0), which additionally should allow us to use uint32_t for timestamp_t/api_timestamp_t, unless there’s a place doing subtraction with them that I haven’t come across.

@karanlyons
Copy link
Contributor Author

So the assumption that timeseries won’t ever contain t=0 is a bit pervasive, to the point where I even relied on it in my last pull. I think I’ve got that solved mostly, though.

As for changing timestamp_t/api_timestamp_t to uint32_t, apart from having to include stdint.h this wasn’t much of a change, and this frees up a bunch of headroom for the timestamps, and I don’t think this module ever supported negative timestamps anyway. I may be forgetting some portability issue with this type declaration, though.

Presuming that these two fixes are made, I’d say the code and documentation should be changed to be unit agnostic with regard to timestamps. This would also clear up some confusion seen in other pull requests asking to support milliseconds and whatnot. Technically milliseconds are already supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant