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

[GitBox <gi...@apache.org>]

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



##########
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:
       Yeah, imo implementing `date_trunc('milliseconds', ns_col)` seems like the appropriate solution here; it's expressive but also familiar for users of other sql engines. We can cast the Arrow type internally as part of the truncation, whilst having it still map nicely to the sql notion of a timestamp regardless of the specified unit.




-- 
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:
users@infra.apache.org