You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "chenkovsky (via GitHub)" <gi...@apache.org> on 2025/06/16 12:01:28 UTC
[PR] feat: support fixed size list for array reverse [datafusion]
chenkovsky opened a new pull request, #16423:
URL: https://github.com/apache/datafusion/pull/16423
## Which issue does this PR close?
## Rationale for this change
array_reverse doesn't support fixed size list now.
## What changes are included in this PR?
support fixed size list for array_reverse
## Are these changes tested?
UT
## Are there any user-facing changes?
No
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support fixed size list for array reverse [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #16423:
URL: https://github.com/apache/datafusion/pull/16423#discussion_r2150385634
##########
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) {
if array.is_null(row_index) {
nulls.push(false);
mutable.extend(0, 0, 1);
continue;
} else {
nulls.push(true);
}
let start = row_index * value_length;
let end = start + value_length;
for idx in (start..end).rev() {
mutable.extend(0, idx, idx + 1);
}
}
```
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support fixed size list for array reverse [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #16423:
URL: https://github.com/apache/datafusion/pull/16423#discussion_r2150397306
##########
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;
+ }
+ }
+
+ let data = mutable.freeze();
+ Ok(Arc::new(FixedSizeListArray::try_new(
+ Arc::clone(field),
+ array.value_length(),
+ arrow::array::make_array(data),
+ Some(nulls.into()),
Review Comment:
we can return array.nulls() here instead
##########
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);
Review Comment:
This `has_nulls` would also be changed to true if there is more than one null, can do something like array.null_count > 1
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support fixed size list for array reverse [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #16423:
URL: https://github.com/apache/datafusion/pull/16423#discussion_r2150397306
##########
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;
+ }
+ }
+
+ let data = mutable.freeze();
+ Ok(Arc::new(FixedSizeListArray::try_new(
+ Arc::clone(field),
+ array.value_length(),
+ arrow::array::make_array(data),
+ Some(nulls.into()),
Review Comment:
we can return array.nulls() here instead
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support fixed size list for array reverse [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support fixed size list for array reverse [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead merged PR #16423:
URL: https://github.com/apache/datafusion/pull/16423
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support fixed size list for array reverse [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #16423:
URL: https://github.com/apache/datafusion/pull/16423#discussion_r2150444753
##########
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);
Review Comment:
This `has_nulls` would also be changed to true if there is more than one null, can do something like array.null_count > 1
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: support fixed size list for array reverse [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
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`
--
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-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org