alamb commented on code in PR #5769: URL: https://github.com/apache/arrow-rs/pull/5769#discussion_r1605310263
########## arrow-array/src/arithmetic.rs: ########## @@ -284,6 +287,13 @@ native_type_op!(u32); native_type_op!(u64); native_type_op!(i256, i256::ZERO, i256::ONE, i256::MIN, i256::MAX); +native_type_op!(IntervalDayTime, IntervalDayTime::ZERO, IntervalDayTime::ONE); +native_type_op!( + IntervalMonthDayNano, + IntervalMonthDayNano::ZERO, Review Comment: I wondered if this needed any documentation, but it appears the answer is it already has some nice documentation 👍  ########## arrow-cast/src/cast/mod.rs: ########## @@ -275,11 +275,6 @@ pub fn can_cast_types(from_type: &DataType, to_type: &DataType) -> bool { DayTime => false, MonthDayNano => false, }, - (Int64, Interval(to_type)) => match to_type { Review Comment: This removes direct casting support for `Int64Array` to `IntervalArray` (which presumably was interpreting the integers incorrectly anyways) What should someone do who uses that cast in their code today? Something like ```rust let cast_array = IntervalArray::from_iter( int64_array .values() .map(|v| IntervalDayTime::from_parts(v<<32, v&0xffffffff) .collect() ``` ? ########## arrow-array/src/types.rs: ########## @@ -264,11 +264,11 @@ Each field is independent (e.g. there is no constraint that the quantity of nanoseconds represents less than a day's worth of time). ```text -┌──────────────────────────────┬─────────────┬──────────────┐ -│ Nanos │ Days │ Months │ -│ (64 bits) │ (32 bits) │ (32 bits) │ -└──────────────────────────────┴─────────────┴──────────────┘ - 0 63 95 127 bit offset +┌───────────────┬─────────────┬─────────────────────────────┐ Review Comment: In particular, the changes to `arrow-integration-test/src/lib.rs` in this PR ########## arrow-row/src/fixed.rs: ########## @@ -163,6 +165,44 @@ impl FixedLengthEncoding for f64 { } } +impl FixedLengthEncoding for IntervalDayTime { Review Comment: This adds support for the new structured types in the row encoder 👍 ########## arrow-buffer/src/arith.rs: ########## @@ -0,0 +1,61 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +macro_rules! derive_arith { Review Comment: Could you possibly document this a bit to give future readers about its purpose ? ########## arrow-ord/src/ord.rs: ########## @@ -396,21 +396,25 @@ pub mod tests { #[test] fn test_interval_dict() { - let values = IntervalDayTimeArray::from(vec![1, 0, 2, 5]); + let v1 = IntervalDayTime::new(0, 1); + let v2 = IntervalDayTime::new(0, 2); + let v3 = IntervalDayTime::new(12, 2); + + let values = IntervalDayTimeArray::from(vec![Some(v1), Some(v2), None, Some(v3)]); let keys = Int8Array::from_iter_values([0, 0, 1, 3]); let array1 = DictionaryArray::new(keys, Arc::new(values)); - let values = IntervalDayTimeArray::from(vec![2, 3, 4, 5]); + let values = IntervalDayTimeArray::from(vec![Some(v3), Some(v2), None, Some(v1)]); let keys = Int8Array::from_iter_values([0, 1, 1, 3]); let array2 = DictionaryArray::new(keys, Arc::new(values)); let cmp = build_compare(&array1, &array2).unwrap(); - assert_eq!(Ordering::Less, cmp(0, 0)); - assert_eq!(Ordering::Less, cmp(0, 3)); - assert_eq!(Ordering::Equal, cmp(3, 3)); - assert_eq!(Ordering::Greater, cmp(3, 1)); - assert_eq!(Ordering::Greater, cmp(3, 2)); + assert_eq!(Ordering::Less, cmp(0, 0)); // v1 vs v3 + assert_eq!(Ordering::Equal, cmp(0, 3)); // v1 vs v1 Review Comment: It took me a while to trace through the double layer of indirection here. Looks good ########## arrow-integration-test/src/lib.rs: ########## @@ -387,6 +398,25 @@ pub fn array_from_json( 1 => b.append_value(match value { Value::Number(n) => n.as_i64().unwrap(), Value::String(s) => s.parse().expect("Unable to parse string as i64"), + _ => panic!("Unable to parse {value:?} as number"), Review Comment: The `panic`s look suspicious to me, but the rest of the code follows the same pattern -- 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]
