Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for extract(x from time) / date_part from time types #8693

Merged
merged 3 commits into from
Mar 21, 2024

Conversation

Jefffrey
Copy link
Contributor

Which issue does this PR close?

Closes #8692

Rationale for this change

What changes are included in this PR?

Extend support for extract and date_part to Time32 and Time64 types, for relevant precisions (hours, minutes, seconds, etc.)

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions core Core DataFusion crate labels Dec 31, 2023
@Jefffrey
Copy link
Contributor Author

Depends on apache/arrow-rs#5262

Will keep this in draft until arrow-rs support is merged and available to Datafusion

(Also need to update docs)

@alamb
Copy link
Contributor

alamb commented Dec 31, 2023

Release of arrow is tracked by apache/arrow-rs#5234

@Jefffrey
Copy link
Contributor Author

Pending apache/arrow-rs#5337 which will come as part of arrow-rs v51.0.0 (next release)

@Jefffrey Jefffrey force-pushed the extract_from_time branch from 99f2073 to 8e32ea8 Compare March 19, 2024 21:20
@Jefffrey Jefffrey marked this pull request as ready for review March 19, 2024 21:20
@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) and removed logical-expr Logical plan and expressions physical-expr Physical Expressions core Core DataFusion crate labels Mar 19, 2024
Comment on lines -155 to -157
///
/// # Panics
/// If `array` is not a temporal type such as Timestamp or Date32
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realized my previous comment #9613 (comment) was incorrect about panics since it'll just return an error, so correcting here

query R
SELECT date_part('microsecond', arrow_cast('23:32:50.123456789'::time, 'Time64(Nanosecond)'))
----
50123456.789000005
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just floating point stuff

Not sure if want to look into it further or not

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Jefffrey -- this looks great to me. Thank you for sticking with this.

@@ -189,6 +194,16 @@ fn epoch(array: &dyn Array) -> Result<ArrayRef> {
}
Date32 => as_date32_array(array)?.unary(|x| x as f64 * SECONDS_IN_A_DAY),
Date64 => as_date64_array(array)?.unary(|x| x as f64 / 1_000_f64),
Time32(Second) => as_time32_second_array(array)?.unary(|x| x as f64),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👨‍🍳 👌 -- very nice

@@ -939,6 +939,153 @@ SELECT date_part('nanosecond', '2020-09-08T12:00:12.12345678+00:00')
----
12123456780

# test_date_part_time
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it also worth testing the EXTRACT (...) syntax too ?

like EXTRACT( hour from arrow_cast('23:32:50'::time, 'Time32(Second)')))?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, added 👍

query R
SELECT date_part('microsecond', arrow_cast('23:32:50.123456789'::time, 'Time64(Nanosecond)'))
----
50123456.789000005
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine

@alamb alamb changed the title Initial support for extract(x from time) Support for extract(x from time) / date_part from time types Mar 20, 2024
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again @Jefffrey

@alamb alamb merged commit dc373a3 into apache:main Mar 21, 2024
24 checks passed
@Jefffrey Jefffrey deleted the extract_from_time branch March 21, 2024 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support extract(x from time) for Time32 and Time64 types
2 participants