[GitHub] [arrow] alamb commented on a change in pull request #10005: ARROW-12232: [Rust][DataFusion] Add SQL support for CAST(.. AS Time) millisecond

[GitBox]

alamb commented on a change in pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#discussion_r612363488



##########
File path: rust/datafusion/src/sql/planner.rs
##########
@@ -1498,6 +1498,7 @@ pub fn convert_data_type(sql: &SQLDataType) -> 
Result<DataType> {
         SQLDataType::Double => Ok(DataType::Float64),
         SQLDataType::Char(_) | SQLDataType::Varchar(_) => Ok(DataType::Utf8),
         SQLDataType::Timestamp => Ok(DataType::Timestamp(TimeUnit::Nanosecond, 
None)),
+        SQLDataType::Time => Ok(DataType::Timestamp(TimeUnit::Millisecond, 
None)),

Review comment:
       I thought the sql 'Time' format was for hours in the day (aka no date)
   
   Thus I would expect this to be something like ` 
DataType::Time32(TimeUnit::Millisecond)` or 
`DataType::Time64(TimeUnit::Nanosecond)`
   
   
   I think postgres uses time as only the time in the day as well: 
https://www.postgresql.org/docs/9.1/datatype-datetime.html
   
   > time of day (no date)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org