-
Notifications
You must be signed in to change notification settings - Fork 1.7k
minor: refactor array reverse internals #18445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ use datafusion_expr::{ | |
| ColumnarValue, Documentation, ScalarUDFImpl, Signature, Volatility, | ||
| }; | ||
| use datafusion_macros::user_doc; | ||
| use itertools::Itertools; | ||
| use std::any::Any; | ||
| use std::sync::Arc; | ||
|
|
||
|
|
@@ -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() | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using |
||
| { | ||
| // 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]); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can just have an empty list here since it's null anyway, instead of extending the child array by one element |
||
| 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; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| 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(), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
| )?)) | ||
| } | ||
|
|
||
|
|
@@ -190,19 +182,15 @@ fn fixed_size_array_reverse( | |
| let values = array.values(); | ||
| let original_data = values.to_data(); | ||
| let capacity = Capacities::Array(original_data.len()); | ||
| let mut nulls = vec![]; | ||
| let mut mutable = | ||
| MutableArrayData::with_capacities(vec![&original_data], false, capacity); | ||
| let value_length = array.value_length() as usize; | ||
|
|
||
| for row_index in 0..array.len() { | ||
| // skip the null value | ||
| if array.is_null(row_index) { | ||
| nulls.push(false); | ||
| mutable.extend(0, 0, value_length); | ||
| continue; | ||
| } else { | ||
| nulls.push(true); | ||
| } | ||
| let start = row_index * value_length; | ||
| let end = start + value_length; | ||
|
|
@@ -216,6 +204,6 @@ fn fixed_size_array_reverse( | |
| Arc::clone(field), | ||
| array.value_length(), | ||
| arrow::array::make_array(data), | ||
| Some(nulls.into()), | ||
| array.nulls().cloned(), | ||
| )?)) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused trait bound