eerhardt commented on a change in pull request #7654:
URL: https://github.com/apache/arrow/pull/7654#discussion_r458353882
##########
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:
> I disagree with the statement that this is not correct, but I want to
understand your viewpoint better...
> From looking at your test case, do you mean to suggest that you believe a
conversion to UTC first and then taking the date from the UTC date/time is the
most natural way to infer a date from a DateTimeOffset?
What I'm saying is that at this very moment in time, the exact same number
of days have passed since the Unix Epoch for me and for you, even though we are
in different time zones and our local calendars may be on different days. If
there was a program that did:
```C#
var array = new Date32Array.Builder()
.Append(DateTimeOffset.Now)
.Build();
```
And it ran on both of our machines at the same moment in time, using the
proposed changes we could end up with 2 different values in the `array`. That
is what is not correct, in my opinion. Since we are taking in a
`DateTimeOffset`, we need to respect the `Offset` part of that structure. It is
a significant piece of information.
The Unix Epoch represents a moment in time. `DateTimeOffset` represents a
moment in time. Taking 2 `DateTimeOffset` instances that are equivalent (for
example calling Equals on the above `westernHemisphereTime` and
`easternHemisphereTime` returns `true`) should result in the exact same number
of days since the Unix Epoch.
Now if a user of this library wants to strip the Offset and set it to
`Zero`, then that is up to them. But this library shouldn't be making
assumptions that the user doesn't want the Offset taken into account.
----------------------------------------------------------------
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
