alamb commented on a change in pull request #779:
URL: https://github.com/apache/arrow-rs/pull/779#discussion_r713233616
##########
File path: arrow/src/array/array_primitive.rs
##########
@@ -624,6 +625,22 @@ mod tests {
assert!(arr.is_null(1));
assert_eq!(-5, arr.value(2));
assert_eq!(-5, arr.values()[2]);
+
+ // a month_day_nano interval contains months, days and nanoseconds,
+ // but we do not yet have accessors for the values
Review comment:
Is it worth adding some ticket explaining what type of accessors would
be valuable? Namely `month`, `day` and `nanos`?
##########
File path: arrow/Cargo.toml
##########
@@ -38,7 +38,7 @@ path = "src/lib.rs"
[dependencies]
serde = { version = "1.0", features = ["rc"] }
serde_derive = "1.0"
-serde_json = { version = "1.0", features = ["preserve_order"] }
+serde_json = { version = "1.0", features = ["preserve_order",
"arbitrary_precision"] }
Review comment:
There is often much concern about adding new dependencies to arrow -
however, this feature does not seem to add any new dependencies:
https://github.com/serde-rs/json/blob/master/Cargo.toml#L74
##########
File path: arrow/src/util/display.rs
##########
@@ -106,6 +106,45 @@ macro_rules! make_string_interval_day_time {
}};
}
+macro_rules! make_string_interval_month_day_nano {
+ ($column: ident, $row: ident) => {{
+ let array = $column
+ .as_any()
+ .downcast_ref::<array::IntervalMonthDayNanoArray>()
+ .unwrap();
+
+ let s = if array.is_null($row) {
+ "NULL".to_string()
+ } else {
+ let value: u128 = array.value($row) as u128;
+
+ let months_part: i32 =
Review comment:
it would be nice to break this logic out into accessors, as you say,
rather than have the field extraction in the stringification. But that could be
done as a follow on PR I think
##########
File path: arrow/src/datatypes/datatype.rs
##########
@@ -158,14 +158,22 @@ pub enum TimeUnit {
Nanosecond,
}
-/// YEAR_MONTH or DAY_TIME interval in SQL style.
+/// YEAR_MONTH, DAY_TIME, MonthDayNano interval in SQL style.
Review comment:
```suggestion
/// YEAR_MONTH, DAY_TIME, MONTH_DAY_NANO interval in SQL style.
```
##########
File path: arrow/src/datatypes/native.rs
##########
@@ -293,6 +328,29 @@ impl ArrowNativeType for u64 {
}
}
+impl JsonSerializable for u128 {
Review comment:
What uses `u128`? It seems like this PR only uses `i128`. I may be
missing something
##########
File path: parquet/src/arrow/arrow_writer.rs
##########
@@ -426,6 +426,14 @@ fn write_leaf(
.unwrap();
get_interval_dt_array_slice(array, &indices)
}
+ _ => {
+ return Err(ParquetError::NYI(
+ format!(
+ "Attempting to write an Arrow type {:?} to
parquet that is not yet implemented",
Review comment:
```suggestion
"Attempting to write an Arrow interval type
{:?} to parquet that is not yet implemented",
```
--
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]
