returnString commented on a change in pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#discussion_r613414768
##########
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 think having the truncation also do the cast for those `date_trunc`
calls is quite reasonable, because:
- it's still a `timestamp` in sql parlance, and we make no guarantees about
the underlying unit of a timestamp presently (afaik? open to corrections here!)
- it doesn't result in any unexpected data loss (by virtue of asking for a
truncation in the first place)
In general, for other granularities, it _could_ make sense for `date_trunc`
to internally cast to the most coarse time unit that covers the requested
precision (e.g. `date_trunc('seconds', timestamp_col)` could be converted to
`TimeUnit::Seconds`, but I have no strong opinions there personally 😄
--
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:
[email protected]
