[GitHub] [arrow-datafusion] isidentical commented on a diff in pull request #4156: Support time32 and time64

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

isidentical commented on code in PR #4156:
URL: https://github.com/apache/arrow-datafusion/pull/4156#discussion_r1018530191


##########
datafusion/proto/src/to_proto.rs:
##########
@@ -1188,7 +1188,28 @@ impl TryFrom<&ScalarValue> for protobuf::ScalarValue {
                 })
             }
 
-            datafusion::scalar::ScalarValue::Time64(v) => {
+            // Since the protos only support Time64 and always interpret it to nanosecond accuracy,

Review Comment:
   Question: is there a particular reason why we can't represent these types directly in our serialization format other than backwards compatibility?



##########
datafusion/common/src/scalar.rs:
##########
@@ -2100,15 +2188,16 @@ impl TryFrom<ScalarValue> for i32 {
     }
 }
 
-// special implementation for i64 because of TimeNanosecond
+// special implementation for i64 because of Date64, Time64, Time64 and Timestamp

Review Comment:
   nit: Seems like `Time64` is repeated
   ```suggestion
   // special implementation for i64 because of Date64, Time64, and Timestamp
   ```



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

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

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