Support DictionaryArray in temporal kernels #2623

viirya · 2022-09-01T07:16:10Z

Which issue does this PR close?

Closes #2622.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

arrow/src/compute/kernels/temporal.rs

viirya · 2022-09-01T18:52:48Z

arrow/src/compute/kernels/temporal.rs

+        $iter.into_iter().for_each(|value| {
+            if let Some(value) = value {
+                match $using(value) {
                    Some(dt) => $builder.append_value($convert(dt.$extract_fn())),
                    None => $builder.append_null(),
                }
+            } else {
+                $builder.append_null();
            }
-        }
+        })


This change basically rewrites the macro using ArrayAccessor API. Otherwise the logic is the same.

Previously the macro can call value_as_datetime or value_as_datetime_with_tz APIs on temporal array, but now we need to call as_datetime on the value instead.

viirya · 2022-09-02T02:51:25Z

cc @sunchao

tustvold

Perhaps we could keep the signature the same, but provide a more generic version with a suffix, e.g. _generic. Otherwise this is an ergonomic step-back for the case of a non-dictionary encoded array, which is likely by far the common case

tustvold · 2022-09-02T06:41:59Z

arrow/src/compute/kernels/cast.rs

            |h| h
        )
    } else {
        // No timezone available. Calling `to_string` on the datatime value simply.
-        extract_component_from_array!(array, builder, to_string, value_as_datetime, |h| h)
+        let iter = ArrayIter::new(array);


Why is this ArrayIter needed? The macro calls into_iter?

tustvold · 2022-09-02T06:47:47Z

arrow/src/compute/kernels/temporal.rs

-                $builder.append_null();
-            } else {
-                match $array.$using(i) {
+    ($iter:ident, $builder:ident, $extract_fn:ident, $using:expr, $convert:expr) => {


I have a really hard time following this macro, I really wonder if we can't replace it with generics or at the very least simplify it... Something for a future PR I suspect

+1. It'd be better to improve the readability here.

Yea, I will find some time trying to simplify it. 😅

tustvold · 2022-09-02T06:49:37Z

arrow/src/compute/kernels/temporal.rs

+pub fn year<T, A: ArrayAccessor<Item = T::Native>>(array: A) -> Result<Int32Array>
+where
+    T: ArrowTemporalType + ArrowNumericType,
+    T::Native: ArrowNativeType,


I think this should be implied by the definition of ArrowPrimitiveType

tustvold · 2022-09-02T06:50:35Z

arrow/src/compute/kernels/temporal.rs

+{
+    match array.data_type().clone() {
+        DataType::Dictionary(_, value_type) => {
+            num_days_from_monday_internal::<T, A>(array, value_type.as_ref().clone())


Is this clone really necessary? Can't num_days_from_monday_internal take &DataType

Yea, &DataType is okay. Removed clone.

tustvold · 2022-09-02T06:53:46Z

arrow/src/compute/kernels/temporal.rs

@@ -516,7 +832,7 @@ mod tests {
        let a: PrimitiveArray<Date64Type> =
            vec![Some(1514764800000), None, Some(1550636625000)].into();

-        let b = hour(&a).unwrap();
+        let b = hour::<Date64Type, _>(&a).unwrap();


This loss of automatic type hinting is a bit unfortunate, it feels like a step-back ergonomically for the common case

viirya · 2022-09-02T08:26:33Z

Perhaps we could keep the signature the same, but provide a more generic version with a suffix, e.g. _generic. Otherwise this is an ergonomic step-back for the case of a non-dictionary encoded array, which is likely by far the common case

Yea, I was hesitating between adding _generic version and replacing existing kernel names. Ideally I'd like to provide the kernels of same names but are generic version. But as you seen, it comes along with the loss of automatic type hinting in the end.

I will add _generic suffix to these new kernels and add back the original ones with same signature.

viirya · 2022-09-04T22:36:25Z

Any more comments? @sunchao @tustvold

sunchao

Looks OK. Do you know whether arrow-rs follows Gregorian calendar or Proleptic Gregorian calendar in terms of handling timestamp prior to 1582-10-15?

sunchao · 2022-09-06T15:52:32Z

arrow/src/compute/kernels/temporal.rs

-                $builder.append_null();
-            } else {
-                match $array.$using(i) {
+    ($iter:ident, $builder:ident, $extract_fn:ident, $using:expr, $convert:expr) => {


+1. It'd be better to improve the readability here.

sunchao · 2022-09-06T16:01:18Z

arrow/src/compute/kernels/temporal.rs

+}
+
+/// Extracts the hours of a given temporal array as an array of integers
+pub fn hour_generic<T, A: ArrayAccessor<Item = T::Native>>(array: A) -> Result<Int32Array>


why we need this hour_generic method? I feel we just need hour.

Please see #2623 (comment). I began with a generic version of hour to replace existing one. But it will be loss of automatic type hinting as we always need to specify at least T when calling the generic hour. Seems a step back ergonomically for the common case.

So currently I provide a generic version and keep original hour signature untouched.

sunchao · 2022-09-06T16:07:31Z

arrow/src/compute/kernels/cast.rs

+            builder,
+            to_string,
+            |value| as_datetime::<T>(
+                <i64 as From<<T as ArrowPrimitiveType>::Native>>::from(value)


nit: we can just use <i64 as From<_>>::from(value) and let type inference do the work

sunchao · 2022-09-06T16:12:04Z

arrow/src/compute/kernels/temporal.rs

@@ -171,335 +172,747 @@ pub fn using_chrono_tz_and_utc_naive_date_time(
        .ok()
 }

-/// Extracts the hours of a given temporal array as an array of integers
+/// Extracts the hours of a given temporal primitive array as an array of integers


nit: add comments explaining what exactly are the returned integers, are they within range [0, 24)?

Yea, [0, 23].

sunchao · 2022-09-06T16:12:59Z

arrow/src/compute/kernels/temporal.rs

    }

    Ok(b.finish())
 }

-/// Extracts the quarter of a given temporal array as an array of integers
+/// Extracts the quarter of a given temporal primitive array as an array of integers


ditto: what do the returned integers represent?

It is the quarter within the range of [1, 4].

viirya · 2022-09-06T20:27:51Z

Looks OK. Do you know whether arrow-rs follows Gregorian calendar or Proleptic Gregorian calendar in terms of handling timestamp prior to 1582-10-15?

It is not explicitly mentioned in this crate. But as datetime operation in arrow-rs uses chrono crate which follows ISO 8601. I think it follows Proleptic Gregorian calendar.

sunchao

LGTM except minor nit on documentation

sunchao · 2022-09-06T22:08:30Z

arrow/src/compute/kernels/temporal.rs

    }

    Ok(b.finish())
 }

-/// Extracts the month of a given temporal array as an array of integers
+/// Extracts the month of a given temporal primitive array as an array of integers


nit: is the returned integers 0-based or 1-based? might add a comment. We should follow the methods like num_days_from_monday which have some nice documentation.

It's 1-based. I will add a comment.

viirya · 2022-09-07T07:11:09Z

Thanks for review.

ursabot · 2022-09-07T07:12:31Z

Benchmark runs are scheduled for baseline = d73d78f and contender = c8bf1ca. c8bf1ca is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

viirya added 2 commits August 31, 2022 17:32

Support dictionary array in temporal kernels

496c471

Support dictionary array in temporal kernels

5171909

viirya commented Sep 1, 2022

View reviewed changes

arrow/src/compute/kernels/temporal.rs Outdated Show resolved Hide resolved

github-actions bot added the arrow Changes to the arrow crate label Sep 1, 2022

viirya added 2 commits September 1, 2022 11:31

Prepare for merging conflicts

4f59e38

Merge remote-tracking branch 'upstream/master' into temporal_hour

b6c8a86

viirya commented Sep 1, 2022

View reviewed changes

tustvold reviewed Sep 2, 2022

View reviewed changes

Keep same kernel signature for primitive array

44d3a5c

sunchao reviewed Sep 6, 2022

View reviewed changes

For review

29d36c7

sunchao approved these changes Sep 6, 2022

View reviewed changes

Add doc

6bf3925

viirya merged commit c8bf1ca into apache:master Sep 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support DictionaryArray in temporal kernels #2623

Support DictionaryArray in temporal kernels #2623

viirya commented Sep 1, 2022

viirya Sep 1, 2022 •

edited

viirya commented Sep 2, 2022

tustvold left a comment

tustvold Sep 2, 2022

viirya Sep 2, 2022

tustvold Sep 2, 2022

sunchao Sep 6, 2022

viirya Sep 6, 2022

tustvold Sep 2, 2022

viirya Sep 2, 2022

tustvold Sep 2, 2022

viirya Sep 2, 2022

tustvold Sep 2, 2022

viirya Sep 2, 2022

viirya commented Sep 2, 2022

viirya commented Sep 4, 2022

sunchao left a comment

sunchao Sep 6, 2022

sunchao Sep 6, 2022

viirya Sep 6, 2022

sunchao Sep 6, 2022

sunchao Sep 6, 2022

viirya Sep 6, 2022

sunchao Sep 6, 2022

viirya Sep 6, 2022

viirya commented Sep 6, 2022

sunchao left a comment

sunchao Sep 6, 2022

viirya Sep 6, 2022

viirya commented Sep 7, 2022

ursabot commented Sep 7, 2022

Support DictionaryArray in temporal kernels #2623

Support DictionaryArray in temporal kernels #2623

Conversation

viirya commented Sep 1, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

viirya Sep 1, 2022 • edited

Choose a reason for hiding this comment

viirya commented Sep 2, 2022

tustvold 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

viirya commented Sep 2, 2022

viirya commented Sep 4, 2022

sunchao 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

viirya commented Sep 6, 2022

sunchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Sep 7, 2022

ursabot commented Sep 7, 2022

viirya Sep 1, 2022 •

edited