Repository navigation
Conversation
github-actions
Bot
added
documentation
|
@alamb @kumarUjjawal PTAL. Can we backport this to 56 too? 😁 |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks -- makes sense to me @sdf-jkl
Can you please also update the datatype docs here with this type? https://datafusion.apache.org/user-guide/sql/data_types.html#date-time-types
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @sdf-jkl
Left few comments please take a look.
| { | ||
| Ok(DataType::Time64(TimeUnit::Nanosecond)) | ||
| let data_type = match precision { | ||
| Some(0) => DataType::Time32(TimeUnit::Second), |
There was a problem hiding this comment.
Correct me if I am wrong but TIME(0), TIME(3) and TIME(6) now give Time32(s), Time32(ms) and Time64(us), but DataFusion has no rule to compare two time values with different units, so these columns cannot be compared, joined or IN-listed against plain TIME values?
There was a problem hiding this comment.
maybe that would be a good follow on to track in a separate ticket
| } | ||
| SQLDataType::Date => Ok(DataType::Date32), | ||
| SQLDataType::Time(None, tz_info) => { | ||
| SQLDataType::Time(precision, tz_info) |
There was a problem hiding this comment.
The precision guard and the precision-to-TimeUnit match, including _ => unreachable!(), are copied from the TIMESTAMP arm instead of shared.
|
|
||
| # Column declarations use the same mapping, including WITHOUT TIME ZONE. | ||
| statement ok | ||
| CREATE TABLE times ( |
There was a problem hiding this comment.
The new times table is created but never dropped, so it stays in the session for the rest of this long test file.**
Which issue does this PR close?
Closes #27271.
Rationale for this change
SQL rejects explicit precision on
TIME, so users cannot name second, millisecond, or microsecond time types in casts or column declarations.What changes are included in this PR?
Accept
TIME(p)for the same precisions supported byTIMESTAMP(p):TIME(0)Time32(Second)TIME(3)Time32(Millisecond)TIME(6)Time64(Microsecond)TIME(9)Time64(Nanosecond)Update the SQL type documentation with these mappings.
What is the testing strategy for this PR?
Added cases in
datafusion/sqllogictest/test_files/datetime/timestamps.sltcover casts, fractional-second truncation, column declarations, nulls,WITHOUT TIME ZONE, and rejection of unsupported precisions and time zones. The new casts and table declaration fail without the implementation.Validation passed: 718 SQL crate tests and doctests, 76 datetime/cast SQL test files, all repository lint checks, including full workspace Clippy with all targets and features and the documentation build. Full workspace tests were not run.
Are there any user-facing changes?
Yes,
TIME(0/3/6/9)is now accepted, including withWITHOUT TIME ZONE. PlainTIMEremainsTime64(Nanosecond). Other precisions,TIMETZ, andTIME WITH TIME ZONEremain unsupported.