You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/07/16 21:37:56 UTC
[GitHub] [arrow] eerhardt commented on a change in pull request #7654: ARROW-8581: [C#] Accept and return DateTime from DateXXArray
eerhardt commented on a change in pull request #7654:
URL: https://github.com/apache/arrow/pull/7654#discussion_r456091623
##########
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 believe this should stay `value.ToUnixTimeMilliseconds()`. Same for Date32 above.
##########
File path: csharp/test/Apache.Arrow.Tests/Date32ArrayTests.cs
##########
@@ -20,28 +20,46 @@ namespace Apache.Arrow.Tests
{
public class Date32ArrayTests
{
- public class Set
+ public class GetDate
Review comment:
The `GetDate` method is now obsolete. So this should probably be updated to the new method name.
##########
File path: csharp/src/Apache.Arrow/Arrays/DelegatingArrayBuilder.cs
##########
@@ -0,0 +1,96 @@
+//------------------------------------------------------------------------------
+// <copyright file="DelegatingArrayBuilder.cs" company="Jetstone Asset Management LLP">
+// Copyright (C) Jetstone Asset Management LLP. All rights reserved.
+// Unauthorised copying of this file, via any medium, is strictly prohibited.
+// Proprietary and confidential
+// </copyright>
+// <author>Adam Szmigin</author>
+//------------------------------------------------------------------------------
+
+using System;
+using Apache.Arrow.Memory;
+
+namespace Apache.Arrow
+{
+ /// <summary>
+ /// The <see cref="DelegatingArrayBuilder{T,TArray,TBuilder}"/> class can be used as the base for any array builder
+ /// that needs to delegate most of its functionality to an inner array builder.
+ /// </summary>
+ /// <remarks>
+ /// The typical use case is when an array builder may accept a number of different types as input, but which are
+ /// all internally converted to a single type for assembly into an array.
+ /// </remarks>
+ /// <typeparam name="T">Type of item accepted by inner array builder.</typeparam>
+ /// <typeparam name="TArray">Type of array produced by this (and the inner) builder.</typeparam>
+ /// <typeparam name="TBuilder">Type of builder (see Curiously-Recurring Template Pattern).</typeparam>
+ public abstract class DelegatingArrayBuilder<T, TArray, TBuilder> : IArrowArrayBuilder<TArray, TBuilder>
Review comment:
Do we have to have this extra layer of abstraction? If we don't think TimestampArray could use this base class, then maybe let's wait with this extra base class. We can always refactor it out of `DateArrayBuilder` later, when it provides value.
##########
File path: csharp/test/Apache.Arrow.Tests/Date32ArrayTests.cs
##########
@@ -20,28 +20,46 @@ namespace Apache.Arrow.Tests
{
public class Date32ArrayTests
{
- public class Set
+ public class GetDate
{
[Fact]
- public void SetAndGet()
+ public void SetAndGetNull()
{
- var now = DateTimeOffset.UtcNow;
+ // Arrange
+ var array = new Date32Array.Builder()
+ .AppendNull()
+ .Build();
+
+ // Act
+ var actual = array.GetDateTime(0);
Review comment:
Can we also get tests for `GetDateTimeOffset`?
##########
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;
}
}
- public Date64Array(ArrayData data)
+ public Date64Array(ArrayData data)
: base(data)
{
data.EnsureDataType(ArrowTypeId.Date64);
}
public override void Accept(IArrowArrayVisitor visitor) => Accept(this, visitor);
- public DateTimeOffset? GetDate(int index)
+ [Obsolete("Use `GetDateTimeOffset()` instead")]
+ public DateTimeOffset? GetDate(int index) => GetDateTimeOffset(index);
+
+ /// <summary>
+ /// Get the date at the specified index in the form of a <see cref="DateTime"/> object.
+ /// </summary>
+ /// <remarks>
+ /// The <see cref="DateTime.Kind"/> property of the returned object is set to
+ /// <see cref="DateTimeKind.Unspecified"/>.
+ /// </remarks>
+ /// <param name="index">Index at which to get the date.</param>
+ /// <returns>Returns a <see cref="DateTime"/> object, or <c>null</c> if there is no object at that index.
+ /// </returns>
+ public DateTime? GetDateTime(int index)
{
long? value = GetValue(index);
+ return value.HasValue
+ ? _epoch.AddMilliseconds(value.Value)
+ : default(DateTime?);
+ }
- if (!value.HasValue)
- {
- return default;
- }
-
- return DateTimeOffset.FromUnixTimeMilliseconds(value.Value);
+ /// <summary>
+ /// Get the date at the specified index in the form of a <see cref="DateTimeOffset"/> object.
+ /// </summary>
+ /// <param name="index">Index at which to get the date.</param>
+ /// <returns>Returns a <see cref="DateTimeOffset"/> object, or <c>null</c> if there is no object at that index.
+ /// </returns>
+ public DateTimeOffset? GetDateTimeOffset(int index)
+ {
+ long? value = GetValue(index);
+ return value.HasValue
+ ? new DateTimeOffset(_epoch.AddMilliseconds(value.Value), TimeSpan.Zero)
Review comment:
I believe this should stay `return DateTimeOffset.FromUnixTimeMilliseconds(value.Value);`. Same for Date32 above.
----------------------------------------------------------------
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:
users@infra.apache.org