[ARROW-8581] [C#] Date32/64Array.Builder should accept DateTime, not DateTimeOffset

• Type: Improvement
• Status: Resolved
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 0.17.0
• Fix Version/s: 2.0.0
• Component/s: C#
• Labels: pull-request-available
• External issue URL: https://github.com/apache/arrow/issues/24746
• Assignee: Unassigned
• Reporter: Adam Szmigin
• Created: 24/Apr/20 08:18
• Updated: 11/Jan/23 08:01
• Resolved: 14/Aug/20 23:52

Description

Summary Proposal

The Date32Array.Builder and Date64.Builder classes both accept values of type DateTimeOffset, but this makes it very easy for the user to introduce subtle bugs when they work with the DateTime type in their own code.  This class of bugs could be avoided if these builders were instead typed on DateTime rather than DateTimeOffset.

Details

The danger is introduced by the implicit widening conversion provided by the DateTimeOffset.Implicit(DateTime to DateTimeOffset) operator:
https://docs.microsoft.com/en-us/dotnet/api/system.datetimeoffset.op_implicit?view=netcore-3.1

The important part is this text:

The offset of the resulting DateTimeOffset object depends on the value of the DateTime.Kind property of the dateTime parameter:

• If the value of the DateTime.Kind property is DateTimeKind.Local or DateTimeKind.Unspecified, the date and time of the DateTimeOffset object is set equal to dateTime, and its Offset property is set equal to the offset of the local system's current time zone.

 (Emphasis mine)

 If the user is operating in an environment with a positive GMT offset, it is very easy to write the wrong date to the builder:

Console.WriteLine(TimeZoneInfo.Local.GetUtcOffset(DateTime.UtcNow)); // Bug triggers if > 00:00:00
var builder = new Date32Array.Builder();
builder.Append(new DateTime(2020, 4, 24)); // Kind == DateTimeKind.Unspecified
var allocator = new NativeMemoryAllocator();
Console.WriteLine(builder.Build(allocator).GetDate(0)); // Prints 2020-04-23!

Assume that the user is in the UK (as I am), where the GMT offset on the above date is 1 hour ahead.  This means that the conversion to DateTimeOffset will actually result in a value of 2020-04-23T23:00:00+01:00 being passed to the Append() method.  Arrow then calls ToUnixTimeMilliseconds(), which only considers the date portion of its object, not the time portion or offset.  This means that the number of days gets calculated based on 2020-04-23, not 2020-04-24 as the user thought they were specifying.

If the user chooses to use NodaTime as a "better" date and time-handling library, they will still likely run into the bug if they do the obvious thing:

Console.WriteLine(TimeZoneInfo.Local.GetUtcOffset(DateTime.UtcNow)); // Bug triggers if > 00:00:00
var builder = new Date32Array.Builder();
var ld = new NodaTime.LocalDate(2020, 4, 24);
builder.Append(ld.ToDateTimeUnspecified()); // Kind == DateTimeKind.Unspecified
var allocator = new NativeMemoryAllocator();
Console.WriteLine(builder.Build(allocator).GetDate(0)); // Prints 2020-04-23!

Suggested Improvement

• Change Date32Array.Builder and Date64Array.Builder to specify a TFrom parameter of DateTime, not DateTimeOffset (breaking change).
• Change Date32Array.GetDate() and Date64Array.GetDate() to return a DateTime, not DateTimeOffset (also a breaking change).

 

The conversion method for a Date32Array would then look a bit like this:

private static readonly DateTime Epoch = new DateTime(1970, 1, 1);

protected override int ConvertTo(DateTime value)
{
    return (int)(value - Epoch).TotalDays;
} 

Issue Links

• links to: GitHub Pull Request #7654