ARROW-16660: [C#] Add support for Time32Array and Time64Array #13279

rishabh-rana · 2022-06-01T05:12:25Z

Implemented support without using TImespan in C# as it cannot hold values that have the resolution in microseconds & nanoseconds as specified on the Jira.

github-actions · 2022-06-01T05:12:45Z

https://issues.apache.org/jira/browse/ARROW-16660

rishabh-rana · 2022-06-01T05:21:31Z

Hey, not sure who the maintainers are that look at the C# implementation, but I saw @eerhardt @HashidaTKS being tagged on a closed PR for C#. Can you please review/ suggest people for reviews?

HashidaTKS

Thank you for your pull request!

I have commented on the details.
If these were addressed, this patch looks good to me.

HashidaTKS · 2022-06-02T13:56:20Z

csharp/test/Apache.Arrow.Tests/TestData.cs

@@ -47,6 +47,8 @@ public static RecordBatch CreateSampleRecordBatch(int length, int columnSetCount
                builder.Field(CreateField(DoubleType.Default, i));
                builder.Field(CreateField(Date32Type.Default, i));
                builder.Field(CreateField(Date64Type.Default, i));
+                builder.Field(CreateField(Time32Type.Default, i));
+                builder.Field(CreateField(Time64Type.Default, i));


I think we need to delete these comments.

arrow/csharp/test/Apache.Arrow.Tests/TestData.cs

Lines 66 to 67 in 0556c8b

//builder.Field(CreateField(Time32Type.Default));

//builder.Field(CreateField(Time64Type.Default));

removed these

HashidaTKS · 2022-06-02T14:35:11Z

csharp/src/Apache.Arrow/Arrays/Time32Array.cs

+    /// </summary>
+    public class Time32Array : PrimitiveArray<int>
+    {
+        private static readonly int seconds_in_millisecond = 1_000;


[nits]
How about directly using 1_000 instead of this field as in Time64Array ?
I think a meaning of 1_000 is clear in GetSeconds() and GetMilliSeconds().

made this change

HashidaTKS · 2022-06-02T15:26:10Z

csharp/src/Apache.Arrow/Arrays/Time64Array.cs

+        /// <summary>
+        /// The <see cref="Builder"/> class can be used to fluently build <see cref="Time64Array"/> objects.
+        /// </summary>
+        public class Builder : PrimitiveArrayBuilder<long, long, Time64Array, Builder>


Does Builder need to inherit PrimitiveArrayBuilder<TFrom, TTo, TArray, TBuilder>?
Can we directly use PrimitiveArrayBuilder<T, TArray, TBuilder> (PrimitiveArrayBuilder<long, Time64Array, Builder>) if ConvertTo does nothing?
It is similar for Time32Array.Builder

I initially played around with using Timespan which needed this, but this is not required now. I have made this change

HashidaTKS · 2022-06-04T00:58:25Z

@rishabh-rana Thanks! It looks good to me but I have no right to merge...

@eerhardt Would you please merge this patch if you have no objection?

eerhardt · 2022-06-04T17:45:14Z

Can you also update the following places?

https://github.com/apache/arrow/blame/master/docs/source/status.rst#L52
Remove the skip line here, so we can start covering all the datetime types in the integration tests:

arrow/dev/archery/archery/integration/datagen.py

Lines 1609 to 1610 in 25e0dd4

generate_datetime_case()

.skip_category('C#'),

eerhardt

This looks pretty good, @rishabh-rana. Thanks so much for the contribution!

I just had some nit comments. I think the most important is to turn on the integration tests now that C# supports Time.

eerhardt · 2022-06-04T17:53:40Z

csharp/src/Apache.Arrow/Arrays/Time32Array.cs

+        /// <param name="index">Index at which to get the date.</param>
+        /// <returns>Returns an int, or <c>null</c> if there is no object at that index.
+        /// </returns>
+        public int? GetMilliSeconds(int index)


One thought I have is that we could add some helper methods for .net 6+ that gets a TimeOnly value at each index. This doesn't need to be done in this PR, but I just wanted to get your thoughts on it.

Sure, I think we can do this for Time32Array. I will raise a follow up

eerhardt · 2022-06-04T17:54:53Z

csharp/src/Apache.Arrow/Arrays/Time64Array.cs

+{
+    /// <summary>
+    /// The <see cref="Time64Array"/> class holds an array of longs, where each value is
+    /// stored as the number of seconds/ milliseconds (depending on the Time64Type) since midnight.


Suggested change

/// stored as the number of seconds/ milliseconds (depending on the Time64Type) since midnight.

/// stored as the number of microseconds/nanoseconds (depending on the Time64Type) since midnight.

eerhardt · 2022-06-04T17:56:20Z

csharp/src/Apache.Arrow/Arrays/Time32Array.cs

+namespace Apache.Arrow
+{
+    /// <summary>
+    /// The <see cref="Time32Array"/> class holds an array of ints, where each value is


Suggested change

/// The <see cref="Time32Array"/> class holds an array of ints, where each value is

/// The <see cref="Time32Array"/> class holds an array of <see cref="Int32" />, where each value is

"int" and "long" are C# specific, but the xml doc is for all languages (VB, F#), so we don't use C# specific keywords in xml doc.

eerhardt · 2022-06-04T17:56:43Z

csharp/src/Apache.Arrow/Arrays/Time64Array.cs

+namespace Apache.Arrow
+{
+    /// <summary>
+    /// The <see cref="Time64Array"/> class holds an array of longs, where each value is


Suggested change

/// The <see cref="Time64Array"/> class holds an array of longs, where each value is

/// The <see cref="Time64Array"/> class holds an array of <see cref="Int64" />, where each value is

eerhardt · 2022-06-04T17:58:24Z

csharp/src/Apache.Arrow/Arrays/Time64Array.cs

+
+        public override void Accept(IArrowArrayVisitor visitor) => Accept(this, visitor);
+
+


Suggested change

extra blank space.

eerhardt · 2022-06-04T17:58:47Z

csharp/src/Apache.Arrow/Arrays/Time64Array.cs

+        /// <summary>
+        /// Get the time at the specified index as seconds
+        /// </summary>
+        /// <param name="index">Index at which to get the date.</param>


Suggested change

/// <param name="index">Index at which to get the date.</param>

/// <param name="index">Index at which to get the time.</param>

eerhardt · 2022-06-04T17:59:13Z

csharp/src/Apache.Arrow/Arrays/Time64Array.cs

+        /// <summary>
+        /// Get the time at the specified index as microseconds
+        /// </summary>
+        /// <param name="index">Index at which to get the date.</param>


Suggested change

/// <param name="index">Index at which to get the date.</param>

/// <param name="index">Index at which to get the time.</param>

eerhardt · 2022-06-04T18:00:15Z

csharp/src/Apache.Arrow/Arrays/Time64Array.cs

+                TimeUnit.Second => value * 1_000_000_000,
+                TimeUnit.Millisecond => value * 1_000_000,


Suggested change

TimeUnit.Second => value * 1_000_000_000,

TimeUnit.Millisecond => value * 1_000_000,

According to

arrow/format/Schema.fbs

Lines 218 to 220 in 25e0dd4

/// The integer `bitWidth` depends on the `unit` and must be one of the following:

/// * SECOND and MILLISECOND: 32 bits

/// * MICROSECOND and NANOSECOND: 64 bits

Time64 only supports micro and nano seconds

eerhardt · 2022-06-04T18:01:04Z

csharp/src/Apache.Arrow/Arrays/Time64Array.cs

+                TimeUnit.Second => value * 1_000_000,
+                TimeUnit.Millisecond => value * 1_000,


Suggested change

TimeUnit.Second => value * 1_000_000,

TimeUnit.Millisecond => value * 1_000,

According to

arrow/format/Schema.fbs

Lines 218 to 220 in 25e0dd4

/// The integer `bitWidth` depends on the `unit` and must be one of the following:

/// * SECOND and MILLISECOND: 32 bits

/// * MICROSECOND and NANOSECOND: 64 bits

Time64 only supports micro and nano seconds

eerhardt · 2022-06-04T18:02:31Z

csharp/src/Apache.Arrow/Arrays/Time32Array.cs

+
+        public override void Accept(IArrowArrayVisitor visitor) => Accept(this, visitor);
+
+


Suggested change

extra blank line

rishabh-rana · 2022-06-06T07:37:08Z

@eerhardt Thank you for your comments! I have resolved all and made the changes to the docs and the integration tests as you mentioned

Add support for new time types.

rishabh-rana · 2022-06-13T06:37:39Z

@eerhardt I have added the missing implementation for the Timestamp array which was causing the integration tests to fail. Can you please approve the workflows to run and review?

eerhardt

This looks good to me. Thank you @rishabh-rana for the contribution!

I'm just waiting for the CI to finish successfully and then I will merge.

rishabh-rana · 2022-06-14T02:32:44Z

@eerhardt Thank you for your review! All good to merge now

tustvold · 2022-07-05T15:10:19Z

We are seeing intermittent archery failures which appear to be related to this change, I wonder if someone might be able to help look into this? Ticket here

add support for Time32Array and Time64Array

0556c8b

github-actions bot added the Component: C# label Jun 1, 2022

HashidaTKS reviewed Jun 2, 2022

View reviewed changes

rishabh-rana added 3 commits June 3, 2022 11:18

resolve comments

575a85d

resolve comments

b38f234

fix

93a348f

HashidaTKS approved these changes Jun 4, 2022

View reviewed changes

eerhardt reviewed Jun 4, 2022

View reviewed changes

resolve comments

1c66456

eerhardt and others added 2 commits June 7, 2022 22:58

Fix Integration tests

e59c9e7

Add support for new time types.

add missing implementation of TimestampArray

78e80ff

github-actions bot added the Component: Documentation label Jun 13, 2022

eerhardt approved these changes Jun 13, 2022

View reviewed changes

eerhardt merged commit 6a936c4 into apache:master Jun 14, 2022

andygrove mentioned this pull request Jun 16, 2022

Add validation to RecordBatch for non-nullable fields containing null values apache/arrow-rs#1890

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-16660: [C#] Add support for Time32Array and Time64Array #13279

ARROW-16660: [C#] Add support for Time32Array and Time64Array #13279

rishabh-rana commented Jun 1, 2022 •

edited

Loading

github-actions bot commented Jun 1, 2022

rishabh-rana commented Jun 1, 2022

HashidaTKS left a comment •

edited

Loading

HashidaTKS Jun 2, 2022

rishabh-rana Jun 3, 2022

HashidaTKS Jun 2, 2022

rishabh-rana Jun 3, 2022

HashidaTKS Jun 2, 2022 •

edited

Loading

rishabh-rana Jun 3, 2022

HashidaTKS commented Jun 4, 2022

eerhardt commented Jun 4, 2022

eerhardt left a comment

eerhardt Jun 4, 2022

rishabh-rana Jun 6, 2022

eerhardt Jun 4, 2022

eerhardt Jun 4, 2022

eerhardt Jun 4, 2022

eerhardt Jun 4, 2022

eerhardt Jun 4, 2022

eerhardt Jun 4, 2022

eerhardt Jun 4, 2022

eerhardt Jun 4, 2022

eerhardt Jun 4, 2022

rishabh-rana commented Jun 6, 2022

rishabh-rana commented Jun 13, 2022

eerhardt left a comment

rishabh-rana commented Jun 14, 2022

tustvold commented Jul 5, 2022

	//builder.Field(CreateField(Time32Type.Default));
	//builder.Field(CreateField(Time64Type.Default));

	/// stored as the number of seconds/ milliseconds (depending on the Time64Type) since midnight.
	/// stored as the number of microseconds/nanoseconds (depending on the Time64Type) since midnight.

	/// The <see cref="Time32Array"/> class holds an array of ints, where each value is
	/// The <see cref="Time32Array"/> class holds an array of <see cref="Int32" />, where each value is

	/// The <see cref="Time64Array"/> class holds an array of longs, where each value is
	/// The <see cref="Time64Array"/> class holds an array of <see cref="Int64" />, where each value is


		public override void Accept(IArrowArrayVisitor visitor) => Accept(this, visitor);

	/// <param name="index">Index at which to get the date.</param>
	/// <param name="index">Index at which to get the time.</param>

		TimeUnit.Second => value * 1_000_000_000,
		TimeUnit.Millisecond => value * 1_000_000,

	/// The integer `bitWidth` depends on the `unit` and must be one of the following:
	/// * SECOND and MILLISECOND: 32 bits
	/// * MICROSECOND and NANOSECOND: 64 bits

		TimeUnit.Second => value * 1_000_000,
		TimeUnit.Millisecond => value * 1_000,

ARROW-16660: [C#] Add support for Time32Array and Time64Array #13279

ARROW-16660: [C#] Add support for Time32Array and Time64Array #13279

Conversation

rishabh-rana commented Jun 1, 2022 • edited Loading

github-actions bot commented Jun 1, 2022

rishabh-rana commented Jun 1, 2022

HashidaTKS left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HashidaTKS Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HashidaTKS commented Jun 4, 2022

eerhardt commented Jun 4, 2022

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishabh-rana commented Jun 6, 2022

rishabh-rana commented Jun 13, 2022

eerhardt left a comment

Choose a reason for hiding this comment

rishabh-rana commented Jun 14, 2022

tustvold commented Jul 5, 2022

rishabh-rana commented Jun 1, 2022 •

edited

Loading

HashidaTKS left a comment •

edited

Loading

HashidaTKS Jun 2, 2022 •

edited

Loading