ARROW-12028 ARROW-11940: [Rust][DataFusion] Add TimestampMillisecond support to GROUP BY/hash aggregates by velvia · Pull Request #9773 · apache/arrow

velvia · 2021-03-22T18:58:28Z

This PR adds support for TimestampMillisecondArray to Scalar, GroupByScalar, and hash_aggregate / GROUP BYs to Rust DataFusion, thus fixing 12028. I believe it might also fix 11940, though this needs input from you all.

There are some formatting changes from running cargo fmt.

…ate, GroupScalar, Scalar

velvia · 2021-03-22T19:02:37Z

@nevi-me @andygrove

alamb · 2021-03-23T22:17:55Z

Thanks @velvia -- I plan to review this tomorrow

Dandandan · 2021-03-23T22:34:58Z

rust/datafusion/src/scalar.rs


 use std::{convert::TryFrom, fmt, iter::repeat, sync::Arc};

+use arrow::array::{


These imports don't seem to be necessary? The module already has array::*, ?

Oh they were reformatted by cargo fmt which is what the README told me to run.....

Dandandan · 2021-03-23T22:35:41Z

rust/datafusion/src/physical_plan/hash_join.rs

-            DataType::UInt64 => {
-                equal_rows_elem!(UInt64Array, l, r, left, right)
-            }
+            DataType::Boolean => equal_rows_elem!(BooleanArray, l, r, left, right),


Any idea why the formatting changed? Do you use a recent/stable rust/cargo/rustfmt version?

This is what I'm running, is it too old?
nightly-2020-11-25-x86_64-apple-darwin (default)

Should arrow project have a rust-toolchain to specify what version of Rust to run on?

FWIW, I tried running cargo fmt with +stable on 1.50.0 and it did not generate any new diffs compared to what I have here....

Dandandan · 2021-03-23T22:36:27Z

This looks good @velvia , I added some minor comments.

github-actions · 2021-03-24T01:24:18Z

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

alamb

Thanks @velvia -- overall looks good.

The only thing I think is needed for this PR is tests for grouping on columns with TimestampMillisecond.

alamb · 2021-03-24T17:36:12Z

rust/datafusion/src/physical_plan/group_scalar.rs

            ScalarValue::UInt16(Some(v)) => GroupByScalar::UInt16(*v),
            ScalarValue::UInt32(Some(v)) => GroupByScalar::UInt32(*v),
            ScalarValue::UInt64(Some(v)) => GroupByScalar::UInt64(*v),
+            ScalarValue::TimeMillisecond(Some(v)) => GroupByScalar::TimeMillisecond(*v),


Should we also add support for ScalarValue::TimeMicrosecond and ScalarValue::TimeNanosecond?

Sure thing, I wasn't sure why they weren't added here....

velvia · 2021-03-29T18:02:27Z

@alamb and others: have added a test and I believe addressed comments, hope this is all needed!

alamb

Looks good to me. Thanks @velvia !

alamb · 2021-03-30T11:45:50Z

@velvia the lint job seems to have failed -- looks like the tests need to get formatted

https://github.com/apache/arrow/pull/9773/checks?check_run_id=2221026494

I tried to fix this PR by checking in the result of running cargo fmt locally, but I don't have access to push to urbanlogiq/arrow

velvia · 2021-03-30T22:33:55Z

@alamb pushed the cargo fmt fix. :)

velvia · 2021-03-31T06:47:42Z

It seems the Docker build failed but I can't click on or expand on the details.

alamb · 2021-03-31T11:36:30Z

It seems the Docker build failed but I can't click on or expand on the details.

Yeah, that has been happening recently. I don't think it is related to this PR. I plan to merge this one as soon as the remaining tests pass

@velvia

…s for Timestamp(_,_) # Rationale: If you try and aggregate (via SUM, for example) a column of a timestamp type, DataFusion generates an error: ``` Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed. ``` For example, from IOx ``` > show columns from t; +---------------+--------------+------------+-------------+-----------------------------+-------------+ | table_catalog | table_schema | table_name | column_name | data_type | is_nullable | +---------------+--------------+------------+-------------+-----------------------------+-------------+ | datafusion | public | t | a | Utf8 | NO | | datafusion | public | t | b | Timestamp(Nanosecond, None) | NO | +---------------+--------------+------------+-------------+-----------------------------+-------------+ 2 row in set. Query took 0 seconds. > select sum(b) from t; Plan("Coercion from [Timestamp(Nanosecond, None)] to the signature Uniform(1, [Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64, Float32, Float64]) failed.") ``` # Changes: Add support for aggregating timestamp types and tests for same # Notes Note this is follow on / more fleshing out of the work done in #9773 by @velvia (👋 thanks for adding Timestamps to `ScalarValue`) Supporting AVG on timestamps is tracked by https://issues.apache.org/jira/browse/ARROW-12318. It is more involved (as currently Avg assumes the output type is always F64), and not important for myuse case at the moment. Closes #9970 from alamb/alamb/ARROW-12277-aggregate-timestamps Authored-by: Andrew Lamb <andrew@nerdnetworks.org> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

Evan Chan added 2 commits March 22, 2021 11:53

[datafusion] Add support for TimestampMillisecondArray in hash_aggreg…

fa22808

…ate, GroupScalar, Scalar

cargo fmt

72f915d

github-actions bot added Component: Rust - DataFusion Component: Rust labels Mar 22, 2021

Dandandan reviewed Mar 23, 2021

View reviewed changes

kou changed the title ~~[rust][datafusion] Fix 12028 and 11940: Add TimestampMillisecond support to GROUP BY/hash aggregates~~ ARROW-12028 ARROW-11940: [Rust][DataFusion] Add TimestampMillisecond support to GROUP BY/hash aggregates Mar 24, 2021

alamb reviewed Mar 24, 2021

View reviewed changes

Evan Chan added 2 commits March 25, 2021 18:14

CR feedback

11ab229

Add test for group by timestamp millis

bd5a0f4

alamb approved these changes Mar 29, 2021

View reviewed changes

Oops, cargo fmt

b3cd8dd

alamb closed this in af4d5f4 Mar 31, 2021

velvia deleted the evan/pr-arrow-12028-groupby-tsmillis branch April 6, 2021 18:03

alamb mentioned this pull request Apr 9, 2021

ARROW-12277: [Rust][DataFusion] Implement and test Count/Min/Max aggregates for Timestamp(_,_) #9970

Closed

asfimport mentioned this pull request Mar 31, 2021

[Rust][DataFusion] Unsupported GROUP BY for Timestamp(Millisecond, None) #27857

Closed


		use std::{convert::TryFrom, fmt, iter::repeat, sync::Arc};

		use arrow::array::{

Conversation

velvia commented Mar 22, 2021

Uh oh!

velvia commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Mar 23, 2021

Uh oh!

Dandandan Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

velvia Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Dandandan Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

velvia Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

velvia Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dandandan commented Mar 23, 2021

Uh oh!

github-actions bot commented Mar 24, 2021

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

velvia Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

velvia commented Mar 29, 2021

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Mar 30, 2021

Uh oh!

velvia commented Mar 30, 2021

Uh oh!

velvia commented Mar 31, 2021

Uh oh!

alamb commented Mar 31, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

velvia commented Mar 22, 2021 •

edited

Loading

velvia Mar 24, 2021 •

edited

Loading