ARROW-11055: [Rust] [DataFusion] Support date_trunc function #9040

paveltiunov · 2020-12-29T07:08:30Z

date_trunc SQL function implementation and GROUP BY timestamp support.

github-actions · 2020-12-29T07:45:42Z

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

jhorstmann · 2020-12-30T11:20:33Z

Cool! A few small comments:

In postgres the argument order is the other way around date_trunc('week', timestamp). I haven't compared with other databases, but following postgres makes sense to me most of the time :)
A few unit tests, especially for the week truncation around the beginning/end of a year would be nice. Maybe the test input could be prepared using to_timestamp so the test is easier to review.
In most usages, the date part parameter would be a literal and it might be worthwhile to optimize for that. Doesn't fit that nicely into the current BuiltinScalarFunction infrastructure, so I'm fine with considering that as out of scope for now.

alamb · 2020-12-31T13:17:45Z

The full set of Rust CI tests did not run on this PR :(

Can you please rebase this PR against apache/master to pick up the changes in #9056 so that they do?

I apologize for the inconvenience.

paveltiunov · 2021-01-02T04:43:34Z

Hey @jhorstmann ! Thanks for the review!

In postgres the argument order is the other way around date_trunc('week', timestamp). I haven't compared with other databases, but following postgres makes sense to me most of the time :)

I've swapped arguments order. It's 50/50 among databases however agree Postgres order is a little bit more common.

A few unit tests, especially for the week truncation around the beginning/end of a year would be nice. Maybe the test input could be prepared using to_timestamp so the test is easier to review.

I've added some tests at the beginning of the year. Is it something you were looking for?

In most usages, the date part parameter would be a literal and it might be worthwhile to optimize for that. Doesn't fit that nicely into the current BuiltinScalarFunction infrastructure, so I'm fine with considering that as out of scope for now.

Yep. Agree it's out of scope for this PR.

@alamb I rebased it against the latest master.

codecov-io · 2021-01-02T04:53:49Z

Codecov Report

Merging #9040 (7a1a618) into master (3992456) will increase coverage by 0.00%.
The diff coverage is 79.89%.

@@           Coverage Diff            @@
##           master    #9040    +/-   ##
========================================
  Coverage   82.56%   82.56%            
========================================
  Files         203      203            
  Lines       50042    50236   +194     
========================================
+ Hits        41315    41476   +161     
- Misses       8727     8760    +33

Impacted Files	Coverage Δ
rust/datafusion/src/physical_plan/group_scalar.rs	`67.85% <0.00%> (-2.52%)`	⬇️
rust/datafusion/src/scalar.rs	`56.17% <0.00%> (-2.83%)`	⬇️
rust/arrow/src/csv/reader.rs	`93.15% <55.55%> (-1.34%)`	⬇️
...ust/datafusion/src/physical_plan/hash_aggregate.rs	`86.48% <60.00%> (-0.74%)`	⬇️
rust/datafusion/src/physical_plan/hash_join.rs	`89.53% <66.66%> (-0.41%)`	⬇️
rust/datafusion/src/test/mod.rs	`89.92% <75.00%> (-0.48%)`	⬇️
...tafusion/src/physical_plan/datetime_expressions.rs	`92.60% <90.74%> (-1.66%)`	⬇️
rust/datafusion/src/execution/context.rs	`89.20% <100.00%> (+0.36%)`	⬆️
rust/datafusion/src/physical_plan/functions.rs	`80.00% <100.00%> (+0.65%)`	⬆️
rust/datafusion/src/physical_plan/type_coercion.rs	`98.54% <100.00%> (+0.01%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3992456...7a1a618. Read the comment docs.

alamb · 2021-01-02T12:44:37Z

rust/datafusion/src/physical_plan/datetime_expressions.rs

+            if array.is_null(i) {
+                Ok(0_i64)
+            } else {
+                let date_time = match granularity_array.value(i) {


This list seems consistent with https://www.postgresql.org/docs/9.1/functions-datetime.html#FUNCTIONS-DATETIME-TRUNC 👍 I think it is fine that we don't yeet support some of the more esoteric stuff (like milleniumm')

alamb · 2021-01-02T12:49:15Z

rust/datafusion/src/physical_plan/datetime_expressions.rs

+        string_builder.append_value("week")?;
+
+        ts_builder.append_value("2020-01-01T13:42:29.190855Z")?;
+        truncated_builder.append_value("2019-12-30T00:00:00.000000Z")?;


alamb · 2021-01-02T12:49:42Z

rust/datafusion/src/physical_plan/group_scalar.rs

@@ -35,6 +35,8 @@ pub(crate) enum GroupByScalar {
    Int32(i32),
    Int64(i64),
    Utf8(Box<String>),
+    TimeMicrosecond(i64),
+    TimeNanosecond(i64),


alamb

Thanks @paveltiunov -- this looks like a great initial addition.

The only thing I think might be worthwhile (as a follow on PR) would be a sql level test in https://github.com/apache/arrow/blob/master/rust/datafusion/tests/sql.rs (to ensure everything was hooked up correctly)

Again, thanks again

paveltiunov · 2021-01-02T19:49:08Z

@alamb Gotcha. I actually added SQL test however it went to context.rs. I saw sql.rs test and I wasn't sure what's the best place for these integration tests. It makes sense to move it there. Good to know. Thanks!

`date_trunc` SQL function implementation and GROUP BY timestamp support. Closes apache#9040 from paveltiunov/date-trunc Authored-by: Pavel Tiunov <pavel.tiunov@gmail.com> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

github-actions bot added Component: Rust - DataFusion Component: Rust labels Dec 29, 2020

paveltiunov force-pushed the date-trunc branch from 12914e8 to d12c12f Compare January 2, 2021 04:36

paveltiunov added 7 commits January 2, 2021 00:42

Date trunc implementation

777234e

Date trunc implementation: code format

3d83d92

Date trunc implementation: SQL test

fccb181

Date trunc implementation: SQL test

0442902

Date trunc implementation: review fixes

03caae4

Date trunc implementation: code format

68d0cbc

Date trunc implementation: clippy fix

7a1a618

paveltiunov force-pushed the date-trunc branch from f7b53f2 to 7a1a618 Compare January 2, 2021 08:44

alamb approved these changes Jan 2, 2021

View reviewed changes

alamb closed this in 4b7cdcb Jan 2, 2021

asfimport mentioned this pull request Jan 2, 2021

[Rust] [DataFusion] Support date_trunc function #26970

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-11055: [Rust] [DataFusion] Support date_trunc function #9040

ARROW-11055: [Rust] [DataFusion] Support date_trunc function #9040

paveltiunov commented Dec 29, 2020

github-actions bot commented Dec 29, 2020

jhorstmann commented Dec 30, 2020

alamb commented Dec 31, 2020

paveltiunov commented Jan 2, 2021

codecov-io commented Jan 2, 2021 •

edited

Loading

alamb Jan 2, 2021

alamb Jan 2, 2021

alamb Jan 2, 2021

alamb left a comment

paveltiunov commented Jan 2, 2021

ARROW-11055: [Rust] [DataFusion] Support date_trunc function #9040

ARROW-11055: [Rust] [DataFusion] Support date_trunc function #9040

Conversation

paveltiunov commented Dec 29, 2020

github-actions bot commented Dec 29, 2020

jhorstmann commented Dec 30, 2020

alamb commented Dec 31, 2020

paveltiunov commented Jan 2, 2021

codecov-io commented Jan 2, 2021 • edited Loading

Codecov Report

alamb Jan 2, 2021

Choose a reason for hiding this comment

alamb Jan 2, 2021

Choose a reason for hiding this comment

alamb Jan 2, 2021

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

paveltiunov commented Jan 2, 2021

codecov-io commented Jan 2, 2021 •

edited

Loading