-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: add Time type support to date_trunc function #19640
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
feat: add Time type support to date_trunc function #19640
Conversation
Co-authored-by: Jeffrey Vo <[email protected]>
| _ => { | ||
| return exec_err!( | ||
| "second argument of `date_trunc` must be timestamp scalar or array" | ||
| "second argument of `date_trunc` must be timestamp, time scalar or array" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "second argument of `date_trunc` must be timestamp, time scalar or array" | |
| "second argument of `date_trunc` must be a timestamp/time scalar or an array" |
| datafusion public date_trunc datafusion public date_trunc FUNCTION true Timestamp(Second, None) SCALAR Truncates a timestamp value to a specified precision. date_trunc(precision, expression) | ||
| datafusion public date_trunc datafusion public date_trunc FUNCTION true Timestamp(Second, Some("+TZ")) SCALAR Truncates a timestamp value to a specified precision. date_trunc(precision, expression) | ||
| datafusion public date_trunc datafusion public date_trunc FUNCTION true Date SCALAR Truncates a timestamp or time value to a specified precision. date_trunc(precision, expression) | ||
| datafusion public date_trunc datafusion public date_trunc FUNCTION true String SCALAR Truncates a timestamp or time value to a specified precision. date_trunc(precision, expression) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are those return types correct - Date and String ? Shouldn't they be converted to Timestamp(Nanosecond, None)
I think the reason is here - https://github.com/apache/datafusion/pull/19640/changes#diff-7a6e17bfd876e9e88341d68750224b8a2294d9f401eef2126a49ade1665c336cR228
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these return types or input types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SELECT arrow_typeof(date_trunc('day', to_date('2024-01-15')));
-- Returns: Timestamp(ns)
SELECT arrow_typeof(date_trunc('day', '2024-01-15'));
-- Returns: Timestamp(ns)
Yes, the actual runtime return type is Timestamp(Nanosecond), not Date or String.
The coercion from Date / String to Timestamp happens at planning time, before invoke_with_args is called.
The Date / String entries in information_schema represent the signature variants generated by the coercible API’s implicit coercion sources. However, the return_type method receives the already-coerced type (Timestamp) and returns that.
So the functional behavior is correct, this is how coercible signatures are represented in metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it! Thanks!
|
Thanks @kumarUjjawal & @martin-g |
Which issue does this PR close?
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?