eerhardt commented on a change in pull request #7654:
URL: https://github.com/apache/arrow/pull/7654#discussion_r457560950
##########
File path: csharp/src/Apache.Arrow/Arrays/Date64Array.cs
##########
@@ -15,56 +15,94 @@
using Apache.Arrow.Types;
using System;
-using System.Collections.Generic;
-using Apache.Arrow.Memory;
namespace Apache.Arrow
{
+ /// <summary>
+ /// The <see cref="Date64Array"/> class holds an array of dates in the
<c>Date64</c> format, where each date is
+ /// stored as the number of milliseconds since the dawn of (UNIX) time,
excluding leap seconds.
+ /// </summary>
public class Date64Array: PrimitiveArray<long>
{
+ private static readonly DateTime _epoch = new DateTime(1970, 1, 1, 0,
0, 0, DateTimeKind.Unspecified);
+
public Date64Array(
ArrowBuffer valueBuffer, ArrowBuffer nullBitmapBuffer,
int length, int nullCount, int offset)
: this(new ArrayData(Date64Type.Default, length, nullCount, offset,
new[] { nullBitmapBuffer, valueBuffer }))
{ }
- public class Builder : PrimitiveArrayBuilder<DateTimeOffset, long,
Date64Array, Builder>
+ /// <summary>
+ /// The <see cref="Builder"/> class can be used to fluently build <see
cref="Date64Array"/> objects.
+ /// </summary>
+ public class Builder : DateArrayBuilder<long, Date64Array, Builder>
{
- internal class DateBuilder: PrimitiveArrayBuilder<long,
Date64Array, DateBuilder>
+ private class DateBuilder: PrimitiveArrayBuilder<long,
Date64Array, DateBuilder>
{
protected override Date64Array Build(
ArrowBuffer valueBuffer, ArrowBuffer nullBitmapBuffer,
int length, int nullCount, int offset) =>
new Date64Array(valueBuffer, nullBitmapBuffer, length,
nullCount, offset);
- }
+ }
+ /// <summary>
+ /// Construct a new instance of the <see cref="Builder"/> class.
+ /// </summary>
public Builder() : base(new DateBuilder()) { }
- protected override long ConvertTo(DateTimeOffset value)
+ protected override long Convert(DateTime dateTime)
+ {
+ return (long)(dateTime - _epoch).TotalMilliseconds;
+ }
+
+ protected override long Convert(DateTimeOffset dateTimeOffset)
{
- return value.ToUnixTimeMilliseconds();
+ return (long)(dateTimeOffset.Date - _epoch).TotalMilliseconds;
Review comment:
> However, I'm not sure whether it is such a good idea to make the same
change in Date32Array. My personal opinion is that (dateTime -
_epoch).TotalDays is more readable as a day count than having to make a trip
into milliseconds and then convert again.
This isn't about read-ability, but instead about correctness. The logic
needs to take into account the Offset portion of the `DateTimeOffset`.
The Unix Epoch is defined as `00:00:00 UTC Thursday, 1 January 1970`. At
that specific time, half of the world was on the day `1 January 1970`, and the
other half of the world was still on `31 December 1969`. I can have the same
**moment in time** represented with 2 different DateTimeOffsets - one with a
negative Offset, and one with a positive Offset. If used the proposed
`Date32Array.Builder`, I would get 2 different days. This is not correct.
For example, the following test should pass, but it doesn't:
```C#
[Fact]
public void RespectsOffset()
{
// Arrange
var westernHemisphereTime = new DateTimeOffset(2020, 7, 20,
20, 0, 0, TimeSpan.FromHours(-5));
var easternHemisphereTime = new DateTimeOffset(2020, 7, 21,
10, 0, 0, TimeSpan.FromHours(9));
var array = new Date32Array.Builder()
.Append(westernHemisphereTime)
.Append(easternHemisphereTime)
.Build();
// Act
var actualFirst = array.GetDate(0);
var actualSecond = array.GetDate(1);
// Assert
Assert.NotNull(actualFirst);
Assert.NotNull(actualSecond);
Assert.Equal(actualFirst, actualSecond);
Assert.Equal(new DateTimeOffset(2020, 7, 21, 0, 0, 0,
TimeSpan.Zero), actualFirst);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
