andre-cc-natzka commented on code in PR #4156:
URL: https://github.com/apache/arrow-datafusion/pull/4156#discussion_r1018802183
##########
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:
I don't think so, in fact I would guess we wanted to represent the other
types as well, i.e. protos would support `Time32Second`, `Time32Millisecond`,
`Time64Microsecond`, `Time64Nanosecond`. I didn't want to change the protos
before discussing them with you first. I would suggest the following changes:
```
enum PrimitiveScalarType{
[...]
INTERVAL_MONTHDAYNANO = 29;
BINARY = 25;
LARGE_BINARY = 26;
TIME32 = 27;
TIME64 = 28;
}
```
and
```
message ScalarValue{
oneof value {
[...]
ScalarTime32Value time32_value = 30;
ScalarTime64Value time64_value = 31;
IntervalMonthDayNanoValue interval_month_day_nano = 32;
StructValue struct_value = 33;
}
}
```
with the new messages:
```
message ScalarTime32Value {
oneof value {
int32 time32_second_value = 1;
int32 time32_millisecond_value = 2;
};
}
message ScalarTime64Value {
oneof value {
int64 time64_microsecond_value = 1;
int64 time64_nanosecond_value = 2;
};
}
```
Then, I would change the `to_proto.rs` and `from_proto.rs` files
consistently. What do you think, @isidentical, @alamb, @waitingkuo,
@avantgardnerio?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
