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

Error in time logic: Year is out of valid range: 1400..10000. #5755

Open
alf opened this issue May 9, 2016 · 8 comments
Open

Error in time logic: Year is out of valid range: 1400..10000. #5755

alf opened this issue May 9, 2016 · 8 comments
Labels
Milestone

Comments

@alf
Copy link

alf commented May 9, 2016

It seems that things start breaking if you're handling dates before the year 1400.

The easiest way to provoke this error message is:

r.ISO8601("0001-01-01T00:00:00Z")

It seems this limitation is documented, however the error message does not point to this and google isn't very helpful since the error message seems to be a generic error from a library.

Since it is possible to insert data that contains pre-medieval dates like below, confusion ensues.

r.db("test").table("test").insert({id: 1, date: r.epochTime(-63576576000)})

I stumbled upon this since the .NET RethinkDb.Driver seems to serialize dates differently in different situations. Thus when I insert a new row it uses epochTime, but when I compare the date in a row with a datetime it uses iso8601.

I suggest the following improvements:

  1. The error message is adjusted to include the above link to technical details
  2. Log a warning or raise an error when creating dates outside the limits using epochTime.
  3. Fix the limitation.
@danielmewes
Copy link
Member

I think it's a good idea to forbid storing such times.
Since we currently allow such times to be stored, this would be a compatiblity-breaking change. I think it's not too critical though. We can implement the check in a way that keeps existing entries working, and only fails when using one of the ReQL date construction terms such as r.epochTime.

Marking as a ReQL_proposal for discussion for one of the upcoming releases.

Fixing the limitation is likely harder and I doubt a lot of people will be using ReQL date functions for dates before 1400.

@danielmewes danielmewes added this to the subsequent milestone May 9, 2016
@alf
Copy link
Author

alf commented May 9, 2016

Note that the limitation is problematic in .NET since it's idiomatic to use DateTime.MinValue to indicate that a date has not been set. This translates to 0001-01-01T00:00:00 (note that the timezone is unspecified). Thus your assumption that few people will be using dates before 1400 doesn't quite hold.

@bchavez
Copy link
Contributor

bchavez commented May 9, 2016

Hi @alf, perhaps using a nullable DateTime? type would be better to indicate a date has not been set?

In my humble opinion, it would be equally (if not more) idiomatic in .NET to use a nullable type rather than DateTime.MinValue. A nullable DateTime? type could be used to avoid the more problematic value like DateTime.MinValue that RethinkDB doesn't natively support.

@alf
Copy link
Author

alf commented May 10, 2016

Personally I prefer:

if (lastChecked.AddHours(1) > DateTime.UtcNow) {
   // do some work
}

as opposed to:

if (lastChecked.HasValue && lastChecked.Value.AddHours(1) > DateTime.UtcNow) {
  // do some work
}

While optional values have a place, in this case a reasonable default value is a better choice.

@bchavez
Copy link
Contributor

bchavez commented May 10, 2016

Hi @alf. May I humbly suggest (or you might already know), using the null conditional operator ?. in the C# language when working with nullable types. It can help maintain similarity to your preferred if statement:

DateTime? lastChecked = null;

if (lastChecked?.AddHours(1) > DateTime.Now)
{
    Console.WriteLine("do some work");
}
else
{
    Console.WriteLine("skip work");
}

The only difference is the ? before the .AddHours(1). But nevertheless, thank you for the unit test and protocol traces. I'll try to investigate more what we can do in the drivers.

If I still could offer any advice, guidance, or best practice on this particular subject w.r.t C#/.NET RethinkDB driver it would still be to use nullable types. This is how I ultimately work with issues like this in my applications.

@alf
Copy link
Author

alf commented May 10, 2016

No need to be humble. I'd forgotten about that bit of syntax and I agree that this solves the issue.

@danielmewes
Copy link
Member

Glad to hear that there's a decent work-around.

Given that storing such dates with years before 1400 might still be common in C#, completely disallowing those dates from getting stored is probably a bad idea.

I think we should just improve the error message then.

@npoppeli
Copy link

On May 9, Daniel Mewes wrote "Fixing the limitation is likely harder and I doubt a lot of people will be using ReQL date functions for dates before 1400". That may be true, but when you're working with genealogical data (like I am), even earlier dates are (should be) possible. Other NoSQL databases do not have this restriction, as far as I can tell.

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

No branches or pull requests

4 participants