Support timestamps and steps of less than a day for range/generate_series #12400

Omega359 · 2024-09-09T18:17:08Z

Which issue does this PR close?

Closes #11822

Rationale for this change

Add support for timestamps and steps less than a day for timestamps.

What changes are included in this PR?

Code, tests, doc.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

… timestamp support.

comphead · 2024-09-09T19:37:58Z

datafusion/functions-nested/src/range.rs

@@ -109,8 +121,11 @@ impl ScalarUDFImpl for Range {
        match args[0].data_type() {


we can probably factor our this block to avoid make a modifications twice

There is so much duplication in these UDF's. I think it's best to push the cleanup of that to a separate PR and keep this one as small as possible.

comphead · 2024-09-09T19:38:31Z

datafusion/functions-nested/src/range.rs

+        return exec_err!(
+            "arguments length must be 3 for {}",
+            if include_upper {
+                "generate_series"


Suggested change

"generate_series"

"GENERATE_SERIES"

comphead · 2024-09-09T19:38:50Z

datafusion/functions-nested/src/range.rs

+            if include_upper {
+                "generate_series"
+            } else {
+                "range"


Suggested change

"range"

"RANGE"

comphead · 2024-09-09T19:39:29Z

datafusion/functions-nested/src/range.rs

@@ -394,3 +412,136 @@ fn gen_range_date(args: &[ArrayRef], include_upper: bool) -> Result<ArrayRef> {

    Ok(arr)
 }
+
+fn gen_range_timestamp(args: &[ArrayRef], include_upper: bool) -> Result<ArrayRef> {


Suggested change

fn gen_range_timestamp(args: &[ArrayRef], include_upper: bool) -> Result<ArrayRef> {

fn gen_range_timestamp(args: &[ArrayRef], include_upper_bound: bool) -> Result<ArrayRef> {

comphead · 2024-09-09T19:40:15Z

datafusion/functions-nested/src/range.rs

+
+    // values are timestamps
+    let values_builder = if let Some(start_tz_str) = start_tz_opt {
+        TimestampNanosecondBuilder::new().with_timezone(&**start_tz_str)


that looks somewhat weird, do we really need so many indirections here?

As it was written, yes, it was required. I changed it up to use map_or_else to accomplish the same thing.

datafusion/functions-nested/src/range.rs

comphead · 2024-09-09T19:43:45Z

datafusion/functions-nested/src/range.rs

+        _ => {
+            internal_err!(
+                "Unexpected argument type for {} : {}",
+                if include_upper {


this error block is third time showing up, perhaps we can factor it out

comphead

Thanks @Omega359 great work
Please check some code comments

For tests its also good to have examples like

series of date2 less than date1 with positive interval

fractional intervals - which might be tricky

Omega359 · 2024-09-09T19:57:11Z

Thanks @comphead for the review, much appreciated. I'll go through the proposed changes and comments tonight or tomorrow and push updates.

Omega359 · 2024-09-10T01:57:46Z

For tests its also good to have examples like

series of date2 less than date1 with positive interval

I think that may already be in there for range and generate_series

query ?
select range(TIMESTAMP '1993-03-01', TIMESTAMP '1989-04-01', INTERVAL '1' YEAR)
----
[]

query ?
select generate_series(TIMESTAMP '1993-03-01', TIMESTAMP '1989-04-01', INTERVAL '1' YEAR)
----
[]

fractional intervals - which might be tricky

I added this one - I think this may be along the lines of what you are thinking of:

query ?
select generate_series(arrow_cast('2021-01-01T00:00:00', 'Timestamp(Nanosecond, Some("-05:00"))'), arrow_cast('2021-01-01T06:00:00', 'Timestamp(Nanosecond, Some("-05:00"))'), INTERVAL '1 HOUR 30 MINUTE -5500000000 NANOSECOND');
----

alamb · 2024-09-10T21:00:43Z

I plan to review this as well tomorrow -- thank you @Omega359

alamb

Thank you @Omega359 for this PR and @dmitrybugakov and @comphead for the reviews. I think this PR is good to go from my perspective (I left some optional nit picky comments but I don't think they are required)

I also ran the reproducer from @Abdullahsab3 on #11822 locally on this branch and it appears to work 👌

> select UNNEST(generate_series('2023-01-01 10:00:00'::timestamp,   '2023-01-31 23:59:00'::timestamp, '1 hour'::interval)) as date_time;
+---------------------+
| date_time           |
+---------------------+
| 2023-01-01T10:00:00 |
| 2023-01-01T11:00:00 |
| 2023-01-01T12:00:00 |
| 2023-01-01T13:00:00 |
| 2023-01-01T14:00:00 |
| 2023-01-01T15:00:00 |
| 2023-01-01T16:00:00 |
| 2023-01-01T17:00:00 |
| 2023-01-01T18:00:00 |
| 2023-01-01T19:00:00 |
| 2023-01-01T20:00:00 |
| 2023-01-01T21:00:00 |
| 2023-01-01T22:00:00 |
| 2023-01-01T23:00:00 |
| 2023-01-02T00:00:00 |
| 2023-01-02T01:00:00 |
| 2023-01-02T02:00:00 |
| 2023-01-02T03:00:00 |
| 2023-01-02T04:00:00 |
| 2023-01-02T05:00:00 |
| 2023-01-02T06:00:00 |
| 2023-01-02T07:00:00 |
| 2023-01-02T08:00:00 |
| 2023-01-02T09:00:00 |
| 2023-01-02T10:00:00 |
| 2023-01-02T11:00:00 |
| 2023-01-02T12:00:00 |
| 2023-01-02T13:00:00 |
| 2023-01-02T14:00:00 |
| 2023-01-02T15:00:00 |
| 2023-01-02T16:00:00 |
| 2023-01-02T17:00:00 |
| 2023-01-02T18:00:00 |
| 2023-01-02T19:00:00 |
| 2023-01-02T20:00:00 |
| 2023-01-02T21:00:00 |
| 2023-01-02T22:00:00 |
| 2023-01-02T23:00:00 |
| 2023-01-03T00:00:00 |
| 2023-01-03T01:00:00 |
| .                   |
| .                   |
| .                   |
+---------------------+
734 row(s) fetched. (First 40 displayed. Use --maxrows to adjust)
Elapsed 0.014 seconds.

alamb · 2024-09-11T14:13:59Z

datafusion/functions-nested/src/range.rs

+                make_scalar_function(|args| gen_range_timestamp(args, false))(args)
+            }
+            dt => {
+                exec_err!("unsupported type for RANGE. Expected Int64, Date32 or Timestamp, got: {dt}")


❤️ for a much better error

datafusion/functions-nested/src/range.rs

alamb · 2024-09-11T14:20:59Z

datafusion/functions-nested/src/range.rs

+    Ok(arr)
+}
+
+fn cast_timestamp_arg(


nit:

You could potentially use as_timestamp_nanosecond_array (the datafusion version) from: https://docs.rs/datafusion/latest/datafusion/common/cast/index.html

And then call timezone to get the timezone from the array

I personally found the use of the word cast somewhat confusing as function doesn't actually call the arrow cast kernel, but instead does the rust as_any stuff (which is like a cast in C or Java)

My programming roots are showing through :) I can take a look at that

alamb · 2024-09-11T14:22:07Z

datafusion/sqllogictest/test_files/array.slt

+[2021-01-01T00:00:00, 2021-01-01T01:00:00, 2021-01-01T02:00:00, 2021-01-01T03:00:00, 2021-01-01T04:00:00, 2021-01-01T05:00:00, 2021-01-01T06:00:00, 2021-01-01T07:00:00, 2021-01-01T08:00:00, 2021-01-01T09:00:00, 2021-01-01T10:00:00, 2021-01-01T11:00:00, 2021-01-01T12:00:00, 2021-01-01T13:00:00, 2021-01-01T14:00:00, 2021-01-01T15:00:00]
+
+query ?
+select generate_series('2021-01-01T00:00:00EST'::timestamp, '2021-01-01T15:00:00-12:00'::timestamp, INTERVAL '1' HOUR);


is it worth testing when the end timestamp is not an exact increment?

There is a test for -5.5sec (in nanoseconds) somewhere in that file.

Co-authored-by: Andrew Lamb <[email protected]>

alamb · 2024-09-12T15:23:48Z

🚀 - thanks again @comphead @Omega359 @dmitrybugakov and @Abdullahsab3

Omega359 added 3 commits September 9, 2024 13:35

Support timestamps and steps of less than a day for timestamps.

e9fc1be

Merge remote-tracking branch 'upstream/main' into range_gen_series_11822

2596358

Updated docs for range and generate_series to add additional info wrt…

ab2a913

… timestamp support.

github-actions bot added documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) labels Sep 9, 2024

Omega359 marked this pull request as ready for review September 9, 2024 19:20

demetribu approved these changes Sep 9, 2024

View reviewed changes

comphead reviewed Sep 9, 2024

View reviewed changes

datafusion/functions-nested/src/range.rs Outdated Show resolved Hide resolved

comphead reviewed Sep 9, 2024

View reviewed changes

datafusion/functions-nested/src/range.rs Outdated Show resolved Hide resolved

comphead reviewed Sep 9, 2024

View reviewed changes

datafusion/functions-nested/src/range.rs Outdated Show resolved Hide resolved

comphead reviewed Sep 9, 2024

View reviewed changes

datafusion/functions-nested/src/range.rs Show resolved Hide resolved

comphead reviewed Sep 9, 2024

View reviewed changes

Updates based on code review.

b2004bd

alamb mentioned this pull request Sep 10, 2024

Support of timestamps and steps of less than a day for generate_series #11822

Closed

alamb mentioned this pull request Sep 11, 2024

DataFusion weekly project plan (Andrew Lamb) - Sep 9, 2024 #12391

Closed

5 tasks

alamb approved these changes Sep 11, 2024

View reviewed changes

Cleanup error message

8be72ab

Co-authored-by: Andrew Lamb <[email protected]>

alamb merged commit 1f06308 into apache:main Sep 12, 2024
25 checks passed

alamb mentioned this pull request Sep 16, 2024

DataFusion weekly project plan (Andrew Lamb) - Sep 16, 2024 #12494

Closed

8 tasks

Omega359 deleted the range_gen_series_11822 branch October 6, 2024 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support timestamps and steps of less than a day for range/generate_series #12400

Support timestamps and steps of less than a day for range/generate_series #12400

Omega359 commented Sep 9, 2024

comphead Sep 9, 2024

Omega359 Sep 10, 2024

comphead Sep 9, 2024

comphead Sep 9, 2024

comphead Sep 9, 2024

comphead Sep 9, 2024

Omega359 Sep 10, 2024

comphead Sep 9, 2024

comphead left a comment

Omega359 commented Sep 9, 2024

Omega359 commented Sep 10, 2024

alamb commented Sep 10, 2024 •

edited

Loading

alamb left a comment

alamb Sep 11, 2024

alamb Sep 11, 2024

Omega359 Sep 11, 2024

alamb Sep 11, 2024

Omega359 Sep 11, 2024

alamb commented Sep 12, 2024 •

edited

Loading

		@@ -109,8 +121,11 @@ impl ScalarUDFImpl for Range {
		match args[0].data_type() {

	fn gen_range_timestamp(args: &[ArrayRef], include_upper: bool) -> Result<ArrayRef> {
	fn gen_range_timestamp(args: &[ArrayRef], include_upper_bound: bool) -> Result<ArrayRef> {

Support timestamps and steps of less than a day for range/generate_series #12400

Support timestamps and steps of less than a day for range/generate_series #12400

Conversation

Omega359 commented Sep 9, 2024

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?

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

comphead left a comment

Choose a reason for hiding this comment

Omega359 commented Sep 9, 2024

Omega359 commented Sep 10, 2024

alamb commented Sep 10, 2024 • edited Loading

alamb 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

alamb commented Sep 12, 2024 • edited Loading

alamb commented Sep 10, 2024 •

edited

Loading

alamb commented Sep 12, 2024 •

edited

Loading