-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add to_char
function implementation using chrono formats
#9181
Conversation
Thanks @Omega359 -- I hope to get a chance to review this over the weekend |
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.
Thank you @Omega359 -- like your previous PRs I found this one very well commented, well tested and a joy to review.
I have one question about the handling of scalar / array arguments, which I think a few more tests could help with, but otherwise this PR looks good to me
Thanks again
to_char
function implementation using chrono formats
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
…r the Duration type (which is now an alias for TimeDelta) and this broke the timestamp tests.
…re/to_char_9147 # Conflicts: # datafusion/physical-expr/src/datetime_expressions.rs
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.
Thank you @Omega359 -- this looks really nice to me 👌
Some(date.timestamp_nanos_opt().unwrap()), | ||
None, | ||
), | ||
StringArray::from(vec!["%Y::%m::%d %S::%M::%H %f".to_string()]), |
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.
👍
query error DataFusion error: Execution error: Cast error: Format error | ||
SELECT to_char('2000-02-03'::date, '%X%K'); | ||
|
||
query error DataFusion error: Arrow error: Invalid argument error: column types must match schema types, expected Utf8 but found Null at column index 0 |
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.
this is probably something we could fix in a follow on PR, as a usability papercut. Might be a good first project for someone 🤔
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.
I probably should handle it appropriately now to be honest. Originally I think this just defaulted to the default formatting for the type which to me I think is actually the best of the potential solutions (vs almost the worst now).
…efault format being used.
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.
Thanks again @Omega359 -- I think there is a bug with null handling, but I also think we can handle that as a follow on PR. I will file a ticket
@@ -2657,16 +2657,19 @@ PT123456S | |||
query T | |||
select to_char(arrow_cast(123456, 'Duration(Second)'), null); | |||
---- | |||
NULL | |||
PT123456S |
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.
Postgres seems to think this should be null
postgres=# select to_char(interval '1 day', null);
to_char
---------
(1 row)
Which issue does this PR close?
Closes #9147
Rationale for this change
The ability to format dates, timestamps or durations is a very useful feature.
What changes are included in this PR?
Code, tests, benchmarks, documentation.
Are these changes tested?
Yes
Are there any user-facing changes?
A new function 'to_char' (alias date_format) is available.