jonathanc-n commented on code in PR #16423:
URL: https://github.com/apache/datafusion/pull/16423#discussion_r2150374823
##########
datafusion/functions-nested/src/reverse.rs:
##########
@@ -175,3 +182,45 @@ fn general_array_reverse<O: OffsetSizeTrait +
TryFrom<i64>>(
Some(nulls.into()),
)?))
}
+
+fn fixed_size_array_reverse(
+ array: &FixedSizeListArray,
+ field: &FieldRef,
+) -> Result<ArrayRef> {
+ let values = array.values();
+ let original_data = values.to_data();
+ let capacity = Capacities::Array(original_data.len());
+ let mut nulls = vec![];
Review Comment:
Instead of creating another null array you can use the `.extend_null`
function that is inside `MutableArrayData`
##########
datafusion/functions-nested/src/reverse.rs:
##########
@@ -175,3 +182,45 @@ fn general_array_reverse<O: OffsetSizeTrait +
TryFrom<i64>>(
Some(nulls.into()),
)?))
}
+
+fn fixed_size_array_reverse(
+ array: &FixedSizeListArray,
+ field: &FieldRef,
+) -> Result<ArrayRef> {
+ 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();
+
+ for row_index in 0..(array.len() as i32) {
+ // skip the null value
+ if array.is_null(row_index as usize) {
+ nulls.push(false);
+ mutable.extend(0, 0, 1);
+ continue;
+ } else {
+ nulls.push(true);
+ }
+
+ let start = row_index * value_length;
+ let end = (row_index + 1) * value_length;
+
+ let mut index = end - 1;
+
+ while index >= start {
+ mutable.extend(0, index as usize, index as usize + 1);
+ index -= 1;
+ }
+ }
Review Comment:
This can be refactored, instead of converting to usize later we can convert
this to usize first. `rev` function can also be used for the reverse logic
```suggestion
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) {
mutable.extend_nulls(1);
} else {
let start = row_index * value_length;
let end = start + value_length;
for idx in (start..end).rev() {
mutable.extend(0, idx, idx + 1);
}
}
}
```
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -7853,11 +7853,10 @@ select array_reverse(arrow_cast(make_array(1, 2, 3),
'LargeList(Int64)')), array
----
[3, 2, 1] [1]
-#TODO: support after FixedSizeList type coercion
-#query ??
-#select array_reverse(arrow_cast(make_array(1, 2, 3), 'FixedSizeList(3,
Int64)')), array_reverse(arrow_cast(make_array(1), 'FixedSizeList(1, Int64)'));
-#----
-#[3, 2, 1] [1]
+query ??
+select array_reverse(arrow_cast(make_array(1, 2, 3), 'FixedSizeList(3,
Int64)')), array_reverse(arrow_cast(make_array(1), 'FixedSizeList(1, Int64)'));
+----
+[3, 2, 1] [1]
Review Comment:
Lets add a test for null values as well
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
