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

Refactor TimeStamp class to UNIXTimeStamp #27

Open
DvirDukhan opened this issue Aug 31, 2020 · 6 comments
Open

Refactor TimeStamp class to UNIXTimeStamp #27

DvirDukhan opened this issue Aug 31, 2020 · 6 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@DvirDukhan
Copy link
Collaborator

As a follow up to #20
DateTime should be converted to UNIXTimeStamp so this library API be fully compliant with RedisTimeSeries native operations.
This change will trigger 2.0.x Release
Side effects:

  1. This change will no longer allow users to store DateTime values without losing information since the translation from DateTime to UNIX timestamp and DateTime again will cause losing information resolution.
@DvirDukhan DvirDukhan added good first issue Good for newcomers help wanted Extra attention is needed labels Aug 31, 2020
@shaunsales
Copy link
Contributor

shaunsales commented Sep 1, 2020

Is there currently a proposed solution for this? Personally I think it's a good change to make, I lost quite a bit of time on this issue starting out and I think others might face the same problems.

The workaround that I've implemented so far is very basic, but gets the job done. I only work with the long type and just convert the parameters inline with the extension methods below;

public static long ToUnixMs(this DateTime timestamp)
{
    return new DateTimeOffset(timestamp).ToUnixTimeMilliseconds();
}

public static DateTime FromUnixMs(this long unixMs)
{
    return DateTimeOffset.FromUnixTimeMilliseconds(unixMs).UtcDateTime;
}

It's worth nothing that currently the range of Unix Ms supported by .NET is larger than the range supported by RedisTimeSeries. For example;

var epochMinus1d = new DateTimeOffset(DateTime.UnixEpoch.AddDays(-1)).ToUnixTimeMilliseconds();
Console.WriteLine(epochMinus1d);
// Output: -86400000

var dateTime = DateTimeOffset.FromUnixTimeMilliseconds(epochMinus1d).UtcDateTime;
Console.WriteLine(dateTime);
// Output: 12/31/1969 12:00:00 AM

I've logged an issue regarding negative timestamp support at RedisTimeSeries/RedisTimeSeries#434

Regarding the loss of information, if we clearly outline that RedisTimeSeries is accurate to the resolution of 1ms in the documentation I think that's OK, especially if we no longer throw errors when adding samples at or before the current timestamp. Ideally if we insert the same timestamp multiple times we can choose how it changes the value data, like overwrite, add or throw error etc.

@DvirDukhan
Copy link
Collaborator Author

DvirDukhan commented Sep 1, 2020

The plan is:

  • Change TimeStamp implicit cast from and to DateTime type to use the built-in .Net functions that you showed in your comment. Rename the class to UNIXTimeStamp
  • Modify tests since the assumption that DateTime is the same after setting and getting it in RedisTimeSeries is no longer valid. I think that the preferred way is to modify the project's datatypes Equals function, where needed, instead of refactoring the entire test project.

@shaunsales
Copy link
Contributor

Could we call it RedisTimeStamp, and create some argument range checks. Let me know if you'd like me to do the PR for this - I have some time this week.

@DvirDukhan
Copy link
Collaborator Author

@shaunsales Your help is highly appreciated, thanks.
What are the range checks for? negative timestamps?
I would like the new type name will be as descriptive as possible, so perhaps RTSTimeStamp (RTS = RedisTimeSeries)? As this is not actually a pure Redis datatype.

@shaunsales
Copy link
Contributor

@DvirDukhan I've completed a first pass of implementing the new TsTimeStamp type. It can only be constructed with valid data from either long or DateTime types. For auto-timestamping, I've overloaded methods to not require timestamp parameters and inform the user in the code hints. You can see the commit here: shaunsales/NRedisTimeSeries@dc2b675

On my TODO list is;

  • Create a TsTimeBucket type with range checks
  • Figure out if we want mixed auto and explicit timestamp support in TS.MADD
  • Add better exception tests for all commands

@DvirDukhan
Copy link
Collaborator Author

@shaunsales please open a PR so we can iterate there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants