Uh oh!
There was an error while loading. Please reload this page.
feat: Add support for TIME literal values - #3010
Conversation
Codecov Report
@@ Coverage Diff @@## master #3010 +/- ##
==========================================
- Coverage 85.95% 85.94% -0.01%
==========================================
Files 291 291 Lines 52382 52420 +38 ==========================================
+ Hits 45025 45054 +29 - Misses 7357 7366 +9
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
avantgardnerio
commented
Aug 2, 2022
Thank you SO much for this! I struggled for a few days on this issue. Your help is very appreciated! |
cc19881 to
8062885Compare| SQLDataType::Boolean => Ok(DataType::Boolean), | ||
| SQLDataType::Date => Ok(DataType::Date32), | ||
| SQLDataType::Time => Ok(DataType::Time64(TimeUnit::Millisecond)), | ||
| SQLDataType::Time => Ok(DataType::Time64(TimeUnit::Nanosecond)), |
There was a problem hiding this comment.
This change constitutes a fix as Time64(Millisecond) is invalid. Rather that switch to Time32(Millisecond), I moved to Time64(Nanosecond) as PostgreSQL requires at least microsecond precision per the documentation. Given Nanosecond comes at no additional cost, it seems reasonable to support the increase in precision.
waitingkuo
commented
Aug 15, 2022
thank you @stuartcarnie ! note that time64 cannot be compared yet as there's no implementation in the kernel. |
alamb
left a comment
There was a problem hiding this comment.
Looks great -- thanks @stuartcarnie and @avantgardnerio
| SQLDataType::Boolean => Ok(DataType::Boolean), | ||
| SQLDataType::Date => Ok(DataType::Date32), | ||
| SQLDataType::Time => Ok(DataType::Time64(TimeUnit::Millisecond)), | ||
| SQLDataType::Time => Ok(DataType::Time64(TimeUnit::Nanosecond)), |
ursabot
commented
Aug 15, 2022
Benchmark runs are scheduled for baseline = 49a3b00 and contender = 15a9a4b. 15a9a4b is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
This PR teaches DataFusion how to interpret
TIMEliterals. These values are stored as nanoseconds from midnight using aTime64(Nanosecond).Which issue does this PR close?
Closes#2883.
Rationale for this change
I adjusted the precision of
TIMEtonanosecond, which is different from the draft PR #2884 by @andygrove. The rationale for choosingNanosecondis that PostgreSQLTIMErequires microsecond precision. Microsecond precision requires theTime64(_)type, so it seems safe to increase the precision to Nanosecond with no additional memory requirements.What changes are included in this PR?
Add support for
TIMEliteralsAre there any user-facing changes?
This PR teaches DataFusion about
TIMEliterals and therefore documentation would need to be revised.