Can someone point me to the problem?

Can someone point me to the problem?

I have this code:

var
  dt, dt2: TDateTime;
  v: Variant;
begin
  dt := EncodeDateTime(2015, 1, 1, 12, 0, 0, 0);
  v := VarSQLTimeStampOffsetCreate(dt);
  dt2 := v;

dt2 is 01.01.2015 11:00:00 

After debugging I found the explanation but I have no clue where exactly the bug lies.

VarSQLTimeStampOffsetCreate internally saves the current timezone which is UTC+2 here (CEST). When converting back it uses TTimeZone.Local.ToLocalTime which does "the trick". It looks if the value passed is within a DST and only then applies the DST offset. Since 1.1.2015 is winter time it only adds the +1 for CET and not the +2 that it originally stored.

I know timezones and stuff can make you crazy but in that case it is just converting back and forth and the result is wrong. My guess is that UTCToLocal from SqlTimSt should not use TTimezone.Local.ToLocal since that uses that extra magic. Or it should use it ALSO when creating the value so it would store +1 in that case and not +2.

Edit:
Ok, I guess this function is wrong:

function DateTimeToSQLTimeStampOffset(const DateTime: TDateTime): TSQLTimeStampOffset;
begin
  Result := DateTimeToSQLTimeStampOffset(DateTime, TTimeZone.Local.UtcOffset.Hours, TTimeZone.Local.UtcOffset.Minutes);
end;

if should be:
function DateTimeToSQLTimeStampOffset(const DateTime: TDateTime): TSQLTimeStampOffset;
begin
  Result := DateTimeToSQLTimeStampOffset(DateTime, TTimeZone.Local.GetUtcOffset(DateTime).Hours, TTimeZone.Local.GetUtcOffset(DateTime).Minutes);
end;

Comments

  1. Sounds as though someone confused himself with time zone complexities while working on the problem of conversion back and forth in a single time zone. Obviously, it should have been coded so that the same steps are in both conversions, and only the sequence is reversed.

    ReplyDelete
  2. Your "should be" looks more or less like that in XE8.

    Result := DateTimeToSQLTimeStampOffset(DateTime, TTimeZone.Local.UtcOffset.Hours, TTimeZone.Local.UtcOffset.Minutes);

    doh... need more coffee... it has the same bug.

    ReplyDelete
  3. Have you already reported that in QP?

    ReplyDelete
  4. Uwe Raabe How can I enter something into the QP before I understand what exactly is the problem? That was the entire point of my post and I was asking for feedback.

    ReplyDelete
  5. Stefan Glienke Sorry, I thought your edit made it obvious: `UtcOffset` gives the offset for `Now` while the offset for the given value returned by `GetUtcOffset` would be the correct one. Looks like a proper unit test for this case is missing.

    ReplyDelete
  6. Uwe Raabe It is my guess that this would be the fix but I don't know if that's the correct one as I don't know about the SQL datetimeoffset type and how that stores the timezone offset. From doing a quick check in my SQL management studio it returned me the current datetime with +2 as offset when executing select sysdatetimeoffset() for today but +1 when I changed my clock to january. But I did not convert some past datetime to a datetimeoffset to check what it does then.

    Also since this code depends on some system setting (the current date/time and timezone) I think we are dealing with un(unit)testable code here. (that is why things like moles exist in .net land where you can mock any system function)

    ReplyDelete
  7. Stefan Glienke The fix depends on the spec of the function in question. Either it should use the current daylight saving state, or the daylight saving state of the time being encoded/decoded. At present it takes one choice in one direction and the other choice in the return direction. I couldn't find any documentation to indicate which choice is should be taken. Perhaps it would be clear to somebody with more familiarity of that part of the library.

    ReplyDelete
  8. David Heffernan "The fix depends on the spec of the function in question."

    It more so depends on the spec of the datetimeoffset type. After reading articles like this one: http://blogs.msdn.com/b/bartd/archive/2009/03/31/the-death-of-datetime.aspx I think it should take the offset of the date and not of the current local time setting. ("note that this uses the server’s current time zone offset, which could be inappropriate for historical dates" from the last paragraph)

    "Perhaps it would be clear to somebody with more familiarity of that part of the library." 

    Now guess why I asked here, Sherlock ;)

    ReplyDelete
  9. As I read from the docs TSQLTimeStampOffset represents a local date/time value together with the offsets valid for that particular time stamp resulting from timezone and DST. The DST setting can only be those of the said date/time value. Otherwise it would be dependent on the time the function were called making the result somewhat meaningless. As DST offsets only make sense inside a given date/time range there is no way that any timestamp during German winter time can have an offset originating from DST. Thus using a DST biased offset when creating a timestamp offset for a winter date is just senseless.

    ReplyDelete
  10. Uwe Raabe I suspect that is the case too. However, I'm sure you are familiar with Windows file times being dependent on the prevailing DST state rather than the state corresponding to the file time.

    ReplyDelete
  11. Your fix is better but is still going to be incorrect for some occurrences, as a local time with DST cannot always be mapped to an UTC time stamp (it will map to several during the DST transition)

    The only sane way is to store only UTC, and only have UTC internally, and have the DST/TZ stuff only handled at display time.

    Delphi's TDateTime makes supporting DST "hard by default" though, as it's a local time (should have been made an UTC reference)

    ReplyDelete
  12. Eric Grange I don't store anything. This is code from a unit test that tests custom Variant to TValue conversion and it recently broke :)

    ReplyDelete
  13. I follow Eric on this one. In the past I had also issues with timestamps. Sending them to external systems, or storing them after retrieving them sometimes there was a difference of several hours. The I decided to only use UTC as the internal value no more problems. The DST/TZ stuff is something the presenter should handle.

    ReplyDelete
  14. Stefan Glienke a variable or temporary value of type TDateTime is a (memory) storage, it's enough to introduce ambiguity in terms of DST :/

    Jo Claes Yes. FWIW we patch "Now", FormatDateTime and a few other functions here so that TDateTime is UTC (zulu time) rather than local time. The only "time" at which DST & tz are accounted for are in presenter, and generally when converting to/from string. This brings Delphi inline with pretty much every other language out there.

    ReplyDelete
  15. That sounds like something that I wish I had done. Now, I dread the thought of addressing the issue.

    ReplyDelete

Post a Comment