feat: support month and year interval for date_bin on constant data #5982

NGA-TRAN · 2023-04-12T22:14:07Z

Which issue does this PR close?

Rationale for this change

In IOx, we see users use data_bin with month interval to group data by month for their monthly report. We think it is a very useful feature to support.

What changes are included in this PR?

Since each month does not include the same number of nanoseconds and for date_bin, it also depends on the origin input. This PR mainly add a function to handle month interval for date bin and some related refactoring work

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, date_bin with month and year interval is now working on constant input

datafusion/core/tests/sqllogictests/test_files/timestamps.slt

datafusion/physical-expr/src/datetime_expressions.rs

NGA-TRAN · 2023-04-12T22:20:27Z

datafusion/core/tests/sqllogictests/test_files/timestamps.slt


 # Does not support months for Month-Day-Nano interval
-statement error This feature is not implemented: DATE_BIN stride does not support month intervals
+statement error DataFusion error: This feature is not implemented: DATE_BIN stride does not support combination of month, day and nanosecond intervals


new error message I have added in this PR

datafusion/physical-expr/src/datetime_expressions.rs

NGA-TRAN · 2023-04-12T22:22:32Z

@alamb, @tustvold, @wolffcm

datafusion/physical-expr/src/datetime_expressions.rs

alamb · 2023-04-13T18:50:23Z

The month interval for array input was converted into ScalarValue::IntervalDayTime (days) somewhere. My next PR is to find that place and convert month interval into ScalarValue::IntervalMonthDayNano instead. I think if I do that, they will automatically work with this code

I suspect the code that is do this is the coercion to the type signature defined here:

https://github.com/apache/arrow-datafusion/blob/1646bf6aae1e568e38c5a43e9a44c34e26222341/datafusion/expr/src/function.rs#L474-L494

I think you could move the Interval(MonthDayNano) signatures above the Interval(DayTime) ones

alamb

Looks like a good direction to me @NGA-TRAN -- thank you.

I left some comments that I think will help simplify the code

FYI @berkaysynnada @waitingkuo @mustafasrepo and @metesynnada who have been working in the interval area recently

datafusion/core/tests/sqllogictests/test_files/timestamps.slt

datafusion/physical-expr/src/datetime_expressions.rs

…is not midnight of the first date of the month

NGA-TRAN

Thanks @alamb . I have address your comments. I also fixed a bug I just found. It was about origin the is not first moment of the month

datafusion/core/tests/sqllogictests/test_files/timestamps.slt

…tead of DayTime

NGA-TRAN · 2023-04-14T18:31:18Z

@alamb

I think you could move the Interval(MonthDayNano) signatures above the Interval(DayTime) ones

It works like a charm. This PR now has dateline works for everything (if I do not miss other cases).
To avoid many duplicate tests with and without INTERVAL keyword, I added INTERVAL into tests without it before and remove it for the ones with it. These make the tests more concise and I know all of them work correctly

alamb

Thanks @NGA-TRAN -- I reviewed the PR carefully and it is looking good. I had some questions about the tests, some of which look incorrect -- I think we should resolve the question about incorrect results prior to merging as I think having incorrect results, even if just temporarily, is not a good thing

datafusion/core/tests/sqllogictests/test_files/timestamps.slt

alamb · 2023-04-14T18:47:09Z

datafusion/core/tests/sqllogictests/test_files/timestamps.slt

+
+# 2-month interval in date_bin with default start time
+query P
+select date_bin('2 month', column1)


datafusion/core/tests/sqllogictests/test_files/timestamps.slt

alamb · 2023-04-14T18:47:59Z

datafusion/core/tests/sqllogictests/test_files/timestamps.slt

+2022-03-01T00:00:00
+
+# month interval with start date is end of the month plus some minutes
+# Note the datetime '2022-03-31 00:00:00'. Its bin is NOT '2022-03-31 00:15:00' which is after its time


alamb · 2023-04-14T18:49:41Z

datafusion/core/tests/sqllogictests/test_files/timestamps.slt

+----
+2021-12-31T00:15:00
+2021-12-31T00:15:00
+2021-12-31T00:15:00


I don't understand why the timestamp

'2022-01-02 00:00:00'

would be binned into the prior year (2021)

2021-12-31

The return data is the start of the bin. The bin width is one year. The source data must be inside the bin. Since the origin is '1970-12-31T00:15:00Z', the start of the bins are '1970-12-31T00:15:00Z', '1971-12-31T00:15:00Z', ..., '2021-12-31T00:15:00Z', '2022-12-31T00:15:00Z', ...

'2022-01-02 00:00:00' is between '2021-12-31T00:15:00Z' and '2022-12-31T00:15:00Z' so it must be in the bin that starts with '2021-12-31T00:15:00Z'

datafusion/core/tests/sqllogictests/test_files/timestamps.slt

datafusion/physical-expr/src/datetime_expressions.rs

alamb · 2023-04-14T18:53:42Z

datafusion/physical-expr/src/datetime_expressions.rs

    };

-    let f = |x: Option<i64>| x.map(|x| date_bin_single(stride, x, origin));
+    let f = |x: Option<i64>| x.map(|x| stride.bin(x, origin));


👍 this is looking good

NGA-TRAN

Thanks @alamb. I have explained the test results and addressed your comments

NGA-TRAN · 2023-04-14T19:03:44Z

datafusion/core/tests/sqllogictests/test_files/timestamps.slt

+----
+2021-12-31T00:15:00
+2021-12-31T00:15:00
+2021-12-31T00:15:00


The return data is the start of the bin. The bin width is one year. The source data must be inside the bin. Since the origin is '1970-12-31T00:15:00Z', the start of the bins are '1970-12-31T00:15:00Z', '1971-12-31T00:15:00Z', ..., '2021-12-31T00:15:00Z', '2022-12-31T00:15:00Z', ...

'2022-01-02 00:00:00' is between '2021-12-31T00:15:00Z' and '2022-12-31T00:15:00Z' so it must be in the bin that starts with '2021-12-31T00:15:00Z'

NGA-TRAN · 2023-04-14T19:06:42Z

datafusion/core/tests/sqllogictests/test_files/timestamps.slt

+  (timestamp '2022-01-02 00:00:00'), 
+  (timestamp '2022-02-02 00:00:00'),
+  (timestamp '2022-02-15 00:00:00'),
+  (timestamp '2022-03-31 00:00:00') 


datafusion/core/tests/sqllogictests/test_files/timestamps.slt

datafusion/physical-expr/src/datetime_expressions.rs

…move default origin from the tests

NGA-TRAN · 2023-04-14T19:59:53Z

@alamb The PR is now ready for re-reviewing with all questions answered

alamb

LGTM -- thank you @NGA-TRAN

I took the liberty of merging this PR from main and adding a few test cases / comment clarifications in 0dc4329

Hopefully this is ok and I plan to merge this PR once the tests pass

cc @waitingkuo @stuartcarnie

waitingkuo · 2023-04-16T14:26:08Z

@alamb thank you for pining me. @NGA-TRAN thank you for the great works. the codes looks good to me.

i tested this in your branch, i wonder whether this corner cases make sense

❯ select date_bin('1 month', timestamp '2000-02-29T00:00:00', timestamp '2000-01-31T00:00:00');
+----------------------------------------------------------------------------------+
| datebin(Utf8("1 month"),Utf8("2000-02-29T00:00:00"),Utf8("2000-01-31T00:00:00")) |
+----------------------------------------------------------------------------------+
| 2000-02-29T00:00:00                                                              |
+----------------------------------------------------------------------------------+
1 row in set. Query took 0.003 seconds.

❯ select date_bin('1 month', timestamp '2000-01-31T00:00:00', timestamp '2000-02-29T00:00:00');
+----------------------------------------------------------------------------------+
| datebin(Utf8("1 month"),Utf8("2000-01-31T00:00:00"),Utf8("2000-02-29T00:00:00")) |
+----------------------------------------------------------------------------------+
| 2000-01-29T00:00:00                                                              |
+----------------------------------------------------------------------------------+
1 row in set. Query took 0.003 seconds.

i feel like the mechanism behind isn't consistent. i thought that the second's is 2000-01-31T00:00:00

waitingkuo · 2023-04-16T14:27:52Z

datafusion/physical-expr/src/datetime_expressions.rs

+    if bin_time > source_date {
+        let month_delta = month_delta - stride_months;
+        bin_time = if month_delta < 0 {
+            origin_date - Months::new(month_delta.unsigned_abs() as u32)
+        } else {
+            origin_date + Months::new(month_delta as u32)
+        };
+    }


this makes 2000-01-31T00:00:00 becomes 2000-01-29T00:00:00 in
❯ select date_bin('1 month', timestamp '2000-01-31T00:00:00', timestamp '2000-02-29T00:00:00');

See #5982 (comment)

stuartcarnie

Looks great, @NGA-TRAN!

Not required for this PR, but perhaps something to consider in a followup is that I thought I would experiment with caching the stride function based on the interval input to the date_bin function:

https://github.com/NGA-TRAN/arrow-datafusion/blob/0dc43290387b7eadb24dcf7c9fcd3576ab3f2672/datafusion/physical-expr/src/datetime_expressions.rs#L511

I came up with the following change, that includes a benchmark of date_bin.

For the 1,000 element timestamp array the baseline (uncached):

month-day-nano/date_bin/1000
                        time:   [3.7791 µs 3.7870 µs 3.7958 µs]
                        thrpt:  [263.45 Melem/s 264.06 Melem/s 264.62 Melem/s]

and after the change, the throughput, which is the number of timestamps elements mapped by the function per second, improved consistently by about 28% or from 268 million to 338 million elements per second:

month-day-nano/date_bin/1000
                        time:   [2.9499 µs 2.9529 µs 2.9563 µs]
                        thrpt:  [338.26 Melem/s 338.65 Melem/s 339.00 Melem/s]
                 change:
                        time:   [-22.338% -22.151% -21.964%] (p = 0.00 < 0.05)
                        thrpt:  [+28.147% +28.454% +28.763%]
                        Performance has improved.

Subject: [PATCH] feat: cache stride function
---
Index: datafusion/physical-expr/Cargo.toml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/datafusion/physical-expr/Cargo.toml b/datafusion/physical-expr/Cargo.toml
--- a/datafusion/physical-expr/Cargo.toml	(revision 0dc43290387b7eadb24dcf7c9fcd3576ab3f2672)
+++ b/datafusion/physical-expr/Cargo.toml	(date 1681677085430)
@@ -75,3 +75,7 @@
 [[bench]]
 harness = false
 name = "in_list"
+
+[[bench]]
+harness = false
+name = "date_bin"
Index: datafusion/physical-expr/benches/date_bin.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/datafusion/physical-expr/benches/date_bin.rs b/datafusion/physical-expr/benches/date_bin.rs
new file mode 100644
--- /dev/null	(date 1681679172908)
+++ b/datafusion/physical-expr/benches/date_bin.rs	(date 1681679172908)
@@ -0,0 +1,38 @@
+use arrow_array::TimestampNanosecondArray;
+use criterion::{
+    black_box, criterion_group, criterion_main, BenchmarkId, Criterion, Throughput,
+};
+use datafusion_common::ScalarValue;
+use datafusion_expr::ColumnarValue;
+use datafusion_physical_expr::datetime_expressions::date_bin;
+use std::sync::Arc;
+
+fn bench_mdn(c: &mut Criterion) {
+    let elem_10 =
+        TimestampNanosecondArray::from_iter_values((0..(10 as i64)).into_iter());
+    let elem_1000 =
+        TimestampNanosecondArray::from_iter_values((0..(1000 as i64)).into_iter());
+
+    let mut group = c.benchmark_group("month-day-nano");
+    for elements in [Arc::new(elem_10), Arc::new(elem_1000)].iter() {
+        group.throughput(Throughput::Elements(elements.len() as u64));
+
+        let args = vec![
+            ColumnarValue::Scalar(ScalarValue::new_interval_mdn(0, 1, 0)),
+            ColumnarValue::Array(elements.clone()),
+            ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(Some(1), None)),
+        ];
+
+        group.bench_with_input(
+            BenchmarkId::new("date_bin", elements.len()),
+            &args,
+            |b, args| {
+                b.iter(|| date_bin(args));
+            },
+        );
+    }
+    group.finish();
+}
+
+criterion_group!(benches, bench_mdn);
+criterion_main!(benches);
Index: datafusion/physical-expr/src/datetime_expressions.rs
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/datafusion/physical-expr/src/datetime_expressions.rs b/datafusion/physical-expr/src/datetime_expressions.rs
--- a/datafusion/physical-expr/src/datetime_expressions.rs	(revision 0dc43290387b7eadb24dcf7c9fcd3576ab3f2672)
+++ b/datafusion/physical-expr/src/datetime_expressions.rs	(date 1681679592855)
@@ -428,6 +428,13 @@
             Interval::Months(months) => date_bin_months_interval(*months, source, origin),
         }
     }
+
+    fn interval_fn(&self) -> (i64, fn(i64, i64, i64) -> i64) {
+        match self {
+            Interval::Nanoseconds(nanos) => (*nanos, date_bin_nanos_interval),
+            Interval::Months(months) => (*months, date_bin_months_interval),
+        }
+    }
 }
 
 // Supported intervals:
@@ -508,7 +515,8 @@
         )),
     };
 
-    let f = |x: Option<i64>| x.map(|x| stride.bin(x, origin));
+    let (stride, stride_fn) = stride.interval_fn();
+    let f = |x: Option<i64>| x.map(|x| stride_fn(stride, x, origin));
 
     Ok(match array {
         ColumnarValue::Scalar(ScalarValue::TimestampNanosecond(v, tz_opt)) => {

stuartcarnie · 2023-04-16T19:31:19Z

datafusion/core/tests/sqllogictests/test_files/timestamps.slt

Great test coverage and comments, thanks @NGA-TRAN 💯

datafusion/core/tests/sqllogictests/test_files/timestamps.slt

Co-authored-by: Stuart Carnie <[email protected]>

NGA-TRAN · 2023-04-17T15:54:17Z

@waitingkuo

You have great tests. I agree they look inconsistent. I think we may want to define the behavior here and see whether we want to keep it or not:

I think this one is easy to understand: the origin is Jan 31 so any other month will always be the last day of that month (28, 29, 30, 31 depending on the month and year)

❯ select date_bin('1 month', timestamp '2000-02-29T00:00:00', timestamp '2000-01-31T00:00:00');
+----------------------------------------------------------------------------------+
| datebin(Utf8("1 month"),Utf8("2000-02-29T00:00:00"),Utf8("2000-01-31T00:00:00")) |
+----------------------------------------------------------------------------------+
| 2000-02-29T00:00:00                                                              |
+----------------------------------------------------------------------------------+
1 row in set. Query took 0.003 seconds.

This is is tricky and we need to identify the behavior.

❯ select date_bin('1 month', timestamp '2000-01-31T00:00:00', timestamp '2000-02-29T00:00:00');
+----------------------------------------------------------------------------------+
| datebin(Utf8("1 month"),Utf8("2000-01-31T00:00:00"),Utf8("2000-02-29T00:00:00")) |
+----------------------------------------------------------------------------------+
| 2000-01-29T00:00:00                                                              |
+----------------------------------------------------------------------------------+
1 row in set. Query took 0.003 seconds.

For February, 29 (or 28) is the last day of the month, but for other month is it not the last day of the month. So chromo's Months that we are using in this PR do not consider 29 as the last day of the month and just simple add/minus 30 days which I think makes sense.

❯ select date_bin('1 month', timestamp '2000-01-31T00:00:00', timestamp '2000-03-29T00:00:00');
+----------------------------------------------------------------------------------+
| datebin(Utf8("1 month"),Utf8("2000-01-31T00:00:00"),Utf8("2000-03-29T00:00:00")) |
+----------------------------------------------------------------------------------+
| 2000-01-29T00:00:00                                                              |
+----------------------------------------------------------------------------------+
1 row in set. Query took 0.005 seconds.

So I think this is the behavior is acceptable. What do you, @alamb and @stuartcarnie think?

NGA-TRAN · 2023-04-17T16:15:13Z

I am adding @stuartcarnie 's suggestion and benchmark into this PR

…enckmark" This reverts commit b45d6e0.

NGA-TRAN · 2023-04-17T18:59:51Z

@alamb : I have added @stuartcarnie's the suggestion without the benchmark because I got Audit error if the benchmark is included. I am not sure what is the policy for adding benchmark

stuartcarnie · 2023-04-17T20:06:17Z

That's great, thanks @NGA-TRAN! date_bin is used extensively in IOx, so downstream will definitely appreciate it!

NGA-TRAN · 2023-04-18T17:18:58Z

I have added the tests with origin on 29th of Feb and Mar and interval is a month. I have added explanation for the tests on why the bins are always on the 29th

alamb

Thanks everyone for their comments and help here

feat: support month and year interval for date_bin on constant data

393bf52

github-actions bot added core Core DataFusion crate physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Apr 12, 2023

NGA-TRAN commented Apr 12, 2023

View reviewed changes

chore: update a test output

ce5a3a5

NGA-TRAN commented Apr 13, 2023

View reviewed changes

datafusion/physical-expr/src/datetime_expressions.rs Outdated Show resolved Hide resolved

alamb reviewed Apr 13, 2023

View reviewed changes

refactor: address review comments and also fix a bug for origin that …

b81b272

…is not midnight of the first date of the month

NGA-TRAN commented Apr 13, 2023

View reviewed changes

datafusion/core/tests/sqllogictests/test_files/timestamps.slt Outdated Show resolved Hide resolved

feat: always create IntervalUnit::MonthDayNano for month interval ins…

d3d8c89

…tead of DayTime

github-actions bot added the logical-expr Logical plan and expressions label Apr 14, 2023

alamb reviewed Apr 14, 2023

View reviewed changes

chore: address review comment

403bd5d

NGA-TRAN commented Apr 14, 2023

View reviewed changes

chore: remove duplicate test but on specified default origin. Also re…

7117429

…move default origin from the tests

alamb added 2 commits April 16, 2023 09:24

Merge remote-tracking branch 'apache/main' into ntran/date_bin

ec29f82

Add a few additional test cases and comment clarification

0dc4329

alamb approved these changes Apr 16, 2023

View reviewed changes

waitingkuo reviewed Apr 16, 2023

View reviewed changes

waitingkuo approved these changes Apr 16, 2023

View reviewed changes

stuartcarnie approved these changes Apr 16, 2023

View reviewed changes

alamb and others added 2 commits April 17, 2023 05:58

Merge remote-tracking branch 'apache/main' into ntran/date_bin

75483c0

chore: Apply suggestions from code review

fd66b16

Co-authored-by: Stuart Carnie <[email protected]>

NGA-TRAN added 3 commits April 17, 2023 12:41

chore: use Stuart's interval function for enum and date_bin benckmark

b45d6e0

Revert "chore: use Stuart's interval function for enum and date_bin b…

279b264

…enckmark" This reverts commit b45d6e0.

chore: add stuart's new function back without benchmark

7c7aad5

test: add tests with ordigin on 29th date of the month

f5af8a0

alamb approved these changes Apr 20, 2023

View reviewed changes

alamb merged commit 011adc2 into apache:main Apr 20, 2023

alamb mentioned this pull request Apr 21, 2023

feat: date_bin output type same with input type #6053

Merged

NGA-TRAN mentioned this pull request Jun 2, 2023

fix: error instead of panic when date_bin interval is 0 #6522

Merged

feat: support month and year interval for date_bin on constant data #5982

feat: support month and year interval for date_bin on constant data #5982

Uh oh!

Conversation

NGA-TRAN commented Apr 12, 2023 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NGA-TRAN commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alamb commented Apr 13, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NGA-TRAN left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NGA-TRAN commented Apr 14, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NGA-TRAN left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NGA-TRAN commented Apr 14, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

waitingkuo commented Apr 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartcarnie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

NGA-TRAN commented Apr 12, 2023 •

edited by alamb

Loading

NGA-TRAN commented Apr 12, 2023 •

edited

Loading

NGA-TRAN left a comment •

edited

Loading

stuartcarnie left a comment •

edited

Loading