You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/12 21:45:14 UTC

[GitHub] [arrow] velvia opened a new pull request #10005: [rust][datafusion] ARROW-12232: Add SQL support for CAST(.. AS Time) millisecond

velvia opened a new pull request #10005:
URL: https://github.com/apache/arrow/pull/10005


   According to DataFusion README, the `TIME` SQL type is supported, corresponding to Timestamp millis.  However, trying to CAST a column AS TIME results in "Unsupported" error.  This quick and small PR adds support for this CAST, and adds a test as well.


-- 
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

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

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#issuecomment-818666509


   Thanks for the PR @velvia !


-- 
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

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

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#discussion_r613966804



##########
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:
       > Here's my proposal:
   > add date_trunc('milliseconds', _) -> returns Timestamp(Milliseconds, None)
   > add date_trunc('microseconds', _) -> returns Timestamp(Microseconds, None)
   > Leave implementation of other time periods to a future PR (there is also a question of what type to return)
   > I would also add automatic coercion of Timestamp millis (and micros, and maybe nanos?)
   
   I think that sounds reasonable 
   
   > So the big question (and I'm not sure if this PR is the right forum for this) is which direction should the project be going. There is a mismatch between SQL support, the design to work with one type, and the Arrow support for multiple time resolutions.
   
   I don't think this PR is the right forum -- I think sending something to dev@arrow.apache.com is probably the right place to have that discussion. 
   
   > Have most functions work on nano resolution timestamps, and have automatic coercion of other resolutions to nanos when calling functions etc. This imposes a perf penalty, but maybe an acceptable one.
   
   this is my personal preferred solution , but I think we should gather input from the larger community. 
   
   
   > Oh, there is also a easier solution. Expand return_type() method in functions.rs to allow passing in the input arrays, so that we could return a different type depending on the time granularity argument.
   
   I think this idea is worth exploring some more 
   




-- 
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

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

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#discussion_r612806099



##########
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:
       There might also be some inspiration in postgres's date/time functions: https://www.postgresql.org/docs/current/functions-datetime.html




-- 
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

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

Posted by GitBox <gi...@apache.org>.
returnString commented on pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#issuecomment-819101432


   
   > > One thing to think about is are there SQL types that correspond to timestamp seconds/micros.
   > 
   > Yes, this is a hole in my mind as the Arrow types are more expressive than standard SQL types. Like there is no way now to create a CSV backed table in datafusion and say you want millisecond based timestamps.
   
   Postgres _sort of_ has a notion of explicit timestamp precision for its types that we could borrow, but it's done using the number of decimal places for fractional seconds rather than unit granularity. In practice though, I don't think this gets used much anyways 🤷‍♂️


-- 
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

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

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#discussion_r612805624



##########
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:
       @velvia 
   
   > Is this only for time of the day?
   
   That is my understanding 
   
   > What I really need is a convenient way to cast things toTimestamp(Milliseconds, None) .. what do you recommend?
   
   I recommend adding a new function that takes a string parameter that is interpreted at an Arrow `DataType` and calls the appropriate underlying arrow cast function. This way we can specify the arrow type directly without having to worry about an incomplete mapping of SQL type <--> Arrow type
   
   Perhaps something like
   
   ```
   select arrow_cast(my_col, 'Timestamp(Milliseconds, None)'
   ```
   
   I haven't thought carefully about how we would parse the strings back to `DateTime` but I think we might be able to press some existing serde code into use, perhaps.
   
   What do you think @jorgecarleitao , @seddonm1 , @returnString ?
   




-- 
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

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

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#issuecomment-821113757


   For anyone following along, @velvia  started a mailing list thread: https://lists.apache.org/thread.html/r8df8e698e5d408f8584ada8da7ed59b5357c96895400d80d016f9e6c%40%3Cdev.arrow.apache.org%3E


-- 
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

[GitHub] [arrow] github-actions[bot] commented on pull request #10005: ARROW-12232: [Rust][DataFusion] Add SQL support for CAST(.. AS Time) millisecond

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#issuecomment-818733668


   https://issues.apache.org/jira/browse/ARROW-12232


-- 
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

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

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org

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

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#discussion_r612813368



##########
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:
       @alamb I am not a fan of trying to invent functions that are not in the SQL spec: i.e. `select arrow_cast(my_col, 'Timestamp(Milliseconds, None)'`.
   
   My vote goes to implement the Postgres functions above.




-- 
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

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

Posted by GitBox <gi...@apache.org>.
velvia commented on a change in pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#discussion_r613610055



##########
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:
       @alamb and others: so I found that there is already an existing `date_trunc()` function, which always returns Timestamp(Nanos).  The problem with the above proposal I put up is:
   * `return_type()` in functions.rs is designed to give the return type knowing only the input argument types.  IOW, I cannot produce a different type such as Timestamp(Millis) as I cannot inspect the time granularity value, all I know is that I have args of type utf8 and timestamp(nanos).  
   * The current function signature is `Exact` and always takes in inputs of nanosecond timestamp resolution.  I suppose we can keep that... but should think about what do people do when they have different timestamp resolutions.
   
   Basically, there is a larger issue in Arrow/Datafusion, in that Timestamp columns with different time resolutions are considered different types, and most of the infrastructure is designed for or works best when there is a single output type (and input type), it is much simpler.   Also in the SQL world, for the most part there is just a "Timestamp" type.  `date_trunc()` and other things have been designed to work with nanos, but you can in theory have timestamp columns which have millis, micros, seconds, but they don't have the same support that nanos do.
   
   So the big question (and I'm not sure if this PR is the right forum for this) is which direction should the project be going.  There is a mismatch between SQL support, the design to work with one type, and the Arrow support for multiple time resolutions.
   
   I can think of some possible directions/solutions, in no particular order:
   - Have most functions work on nano resolution timestamps, and have automatic coercion of other resolutions to nanos when calling functions etc.  This imposes a perf penalty, but maybe an acceptable one.
   - Add support for specifying the resolution for timestamps.  For example, `CAST(xx AS TIMESTAMP(Microseconds))` would be possible.  There's probably precent for this somewhere.   This would need the previous coercion though to work.
   - Somehow support just a single timestamp type in Arrow, with multiple possible resolutions.   I'm not sure how this would even be represented or implemented, and it'd be a huge change.  It might match better how things are implemented in other parts of the SQL world though.
   
   Would love everyone's thoughts on this.   For now I'd be super happy with the first two solutions.




-- 
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

[GitHub] [arrow] velvia commented on pull request #10005: [rust][datafusion] ARROW-12232: Add SQL support for CAST(.. AS Time) millisecond

Posted by GitBox <gi...@apache.org>.
velvia commented on pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#issuecomment-818264049


   @alamb here's a quick one that's slightly related to the one you opened.
   One thing to think about is are there SQL types that correspond to timestamp seconds/micros.


-- 
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

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

Posted by GitBox <gi...@apache.org>.
velvia commented on a change in pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#discussion_r613610417



##########
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:
       Oh, there is also a easier solution.  Expand `return_type()` method in functions.rs to allow passing in the input arrays, so that we could return a different type depending on the time granularity argument.




-- 
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

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

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#issuecomment-820331369


   Looks good -- thanks @velvia  -- I'll close this PR for now to keep the list clean. Feel free to reopen it. 


-- 
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

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

Posted by 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

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

Posted by GitBox <gi...@apache.org>.
velvia commented on pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#issuecomment-819823846


   @returnString @alamb I've filed a new Arrow jira:
   https://issues.apache.org/jira/browse/ARROW-12391
   
   which covers date_trunc, and will start working on that.  This PR, I can close or come back to to solve for the time datatype, it should be trivial in either case.


-- 
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

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

Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#discussion_r613154343



##########
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:
       > date_trunc('milliseconds', ns_col) 
   
   sounds like a good idea to me




-- 
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

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

Posted by GitBox <gi...@apache.org>.
velvia commented on a change in pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#discussion_r612551640



##########
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:
       @alamb according to the DataFusion README, this shows up:
   ```
   TIME | Time64(TimeUnit::Millisecond)
   -- | --
   ```
   Is this only for time of the day?
   
   Basically, we have datasets in both nanos and millis, and don't really care for the extra precision of nanos, and a whole bunch of processing code tuned towards millis, so would rather convert everything to millis for convenience.  What I really need is a convenient way to cast things to millis, or precisely `Timestamp(Milliseconds, None)`, that works with the SQL parser... what do you recommend?
   
   I'm not sure there is a SQL convention for precision of timestamps like in Arrow.  I see some places that have decimal places of precision, which doesn't really map well to our scheme either.  We could invent something different, like `TIMESTAMP(precision)` or allow casting to `TIMESTAMP(Nanos/Millis/Micros)` etc which would solve the problem... WDYT?
   




-- 
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

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

Posted by GitBox <gi...@apache.org>.
velvia commented on a change in pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#discussion_r612552162



##########
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:
       (btw I have a similar problem with the `to_timestamp()` function too, for which I can create another PR)




-- 
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

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

Posted by GitBox <gi...@apache.org>.
velvia commented on a change in pull request #10005:
URL: https://github.com/apache/arrow/pull/10005#discussion_r613406501



##########
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:
       Hmmm, I'm def in favour of implementing PostGres compatible support, though there is a lot there, especially with intervals and timestamps, and many operators including math operators to add.
   
   As for `date_trunc()`, my read of the PostGres functionality is that it is used for date _rounding_, like I can round something to the nearest hour or minute.   This is something definitely useful to add, though I'm more interested in casting to a specific precision.
   
   Here's my proposal:
   * add `date_trunc('milliseconds', _)` -> returns `Timestamp(Milliseconds, None)`
   * add `date_trunc('microseconds', _)` -> returns `Timestamp(Microseconds, None)`
   * Leave implementation of other time periods to a future PR (there is also a question of what type to return)
   * I would also add automatic coercion of Timestamp millis (and micros, and maybe nanos?) to Int64 for fast and easy Int64 comparison, and coercion to nanos for when comparison with nano timestamps matters.
   
   The issue with return types would need to be solved, but the above proposal would work for our needs.
   I can also update the bug once we decide on the solution.
   
   thanks,
   Evan




-- 
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

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

Posted by GitBox <gi...@apache.org>.
alamb closed pull request #10005:
URL: https://github.com/apache/arrow/pull/10005


   


-- 
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

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

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org