Jefffrey commented on code in PR #18445:
URL: https://github.com/apache/datafusion/pull/18445#discussion_r2484815503
##########
datafusion/functions-nested/src/reverse.rs:
##########
@@ -137,49 +138,40 @@ pub fn array_reverse_inner(arg: &[ArrayRef]) ->
Result<ArrayRef> {
}
}
-fn general_array_reverse<O: OffsetSizeTrait + TryFrom<i64>>(
+fn general_array_reverse<O: OffsetSizeTrait>(
array: &GenericListArray<O>,
field: &FieldRef,
) -> Result<ArrayRef> {
let values = array.values();
let original_data = values.to_data();
let capacity = Capacities::Array(original_data.len());
let mut offsets = vec![O::usize_as(0)];
- let mut nulls = vec![];
let mut mutable =
MutableArrayData::with_capacities(vec![&original_data], false,
capacity);
- for (row_index, offset_window) in array.offsets().windows(2).enumerate() {
+ for (row_index, (&start, &end)) in
array.offsets().iter().tuple_windows().enumerate()
+ {
// skip the null value
if array.is_null(row_index) {
- nulls.push(false);
- offsets.push(offsets[row_index] + O::one());
- mutable.extend(0, 0, 1);
+ offsets.push(offsets[row_index]);
continue;
- } else {
- nulls.push(true);
}
- let start = offset_window[0];
- let end = offset_window[1];
-
let mut index = end - O::one();
- let mut cnt = 0;
-
while index >= start {
mutable.extend(0, index.to_usize().unwrap(),
index.to_usize().unwrap() + 1);
index = index - O::one();
- cnt += 1;
}
- offsets.push(offsets[row_index] + O::usize_as(cnt));
+ let size = end - start;
+ offsets.push(offsets[row_index] + size);
}
let data = mutable.freeze();
Ok(Arc::new(GenericListArray::<O>::try_new(
Arc::clone(field),
OffsetBuffer::<O>::new(offsets.into()),
arrow::array::make_array(data),
- Some(nulls.into()),
+ array.nulls().cloned(),
Review Comment:
We can directly copy the nulls from the input array instead of recreating it
step by step (do the same for fixed size lists below)
##########
datafusion/functions-nested/src/reverse.rs:
##########
@@ -137,49 +138,40 @@ pub fn array_reverse_inner(arg: &[ArrayRef]) ->
Result<ArrayRef> {
}
}
-fn general_array_reverse<O: OffsetSizeTrait + TryFrom<i64>>(
+fn general_array_reverse<O: OffsetSizeTrait>(
array: &GenericListArray<O>,
field: &FieldRef,
) -> Result<ArrayRef> {
let values = array.values();
let original_data = values.to_data();
let capacity = Capacities::Array(original_data.len());
let mut offsets = vec![O::usize_as(0)];
- let mut nulls = vec![];
let mut mutable =
MutableArrayData::with_capacities(vec![&original_data], false,
capacity);
- for (row_index, offset_window) in array.offsets().windows(2).enumerate() {
+ for (row_index, (&start, &end)) in
array.offsets().iter().tuple_windows().enumerate()
Review Comment:
Using `tuple_windows` from itertools for a nicer interface on grabbing the
start & end indices
##########
datafusion/functions-nested/src/reverse.rs:
##########
@@ -137,49 +138,40 @@ pub fn array_reverse_inner(arg: &[ArrayRef]) ->
Result<ArrayRef> {
}
}
-fn general_array_reverse<O: OffsetSizeTrait + TryFrom<i64>>(
+fn general_array_reverse<O: OffsetSizeTrait>(
array: &GenericListArray<O>,
field: &FieldRef,
) -> Result<ArrayRef> {
let values = array.values();
let original_data = values.to_data();
let capacity = Capacities::Array(original_data.len());
let mut offsets = vec![O::usize_as(0)];
- let mut nulls = vec![];
let mut mutable =
MutableArrayData::with_capacities(vec![&original_data], false,
capacity);
- for (row_index, offset_window) in array.offsets().windows(2).enumerate() {
+ for (row_index, (&start, &end)) in
array.offsets().iter().tuple_windows().enumerate()
+ {
// skip the null value
if array.is_null(row_index) {
- nulls.push(false);
- offsets.push(offsets[row_index] + O::one());
- mutable.extend(0, 0, 1);
+ offsets.push(offsets[row_index]);
continue;
- } else {
- nulls.push(true);
}
- let start = offset_window[0];
- let end = offset_window[1];
-
let mut index = end - O::one();
- let mut cnt = 0;
Review Comment:
`cnt` can be determined once up front
##########
datafusion/functions-nested/src/reverse.rs:
##########
@@ -137,49 +138,40 @@ pub fn array_reverse_inner(arg: &[ArrayRef]) ->
Result<ArrayRef> {
}
}
-fn general_array_reverse<O: OffsetSizeTrait + TryFrom<i64>>(
+fn general_array_reverse<O: OffsetSizeTrait>(
Review Comment:
Unused trait bound
##########
datafusion/functions-nested/src/reverse.rs:
##########
@@ -137,49 +138,40 @@ pub fn array_reverse_inner(arg: &[ArrayRef]) ->
Result<ArrayRef> {
}
}
-fn general_array_reverse<O: OffsetSizeTrait + TryFrom<i64>>(
+fn general_array_reverse<O: OffsetSizeTrait>(
array: &GenericListArray<O>,
field: &FieldRef,
) -> Result<ArrayRef> {
let values = array.values();
let original_data = values.to_data();
let capacity = Capacities::Array(original_data.len());
let mut offsets = vec![O::usize_as(0)];
- let mut nulls = vec![];
let mut mutable =
MutableArrayData::with_capacities(vec![&original_data], false,
capacity);
- for (row_index, offset_window) in array.offsets().windows(2).enumerate() {
+ for (row_index, (&start, &end)) in
array.offsets().iter().tuple_windows().enumerate()
+ {
// skip the null value
if array.is_null(row_index) {
- nulls.push(false);
- offsets.push(offsets[row_index] + O::one());
- mutable.extend(0, 0, 1);
+ offsets.push(offsets[row_index]);
Review Comment:
We can just have an empty list here since it's null anyway, instead of
extending the child array by one element
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
