You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jkosh44 (via GitHub)" <gi...@apache.org> on 2025/02/04 18:57:18 UTC
[PR] bug: Fix edge cases in array_slice [datafusion]
jkosh44 opened a new pull request, #14489:
URL: https://github.com/apache/datafusion/pull/14489
This commit fixes the following edge cases in the array_slice function so that it's semantics match DuckDB:
- When begin < 0 and -begin > length, begin is clamped to the beginning of the list.
- When step < 0 and begin = end, then the result should be a list with the single element found at index begin/end.
Fixes #10548
## Which issue does this PR close?
Closes #10548.
## Are these changes tested?
Yes
## Are there any user-facing changes?
Yes, the behavior of `array_slice` has changed.
--
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] bug: Fix edge cases in array_slice [datafusion]
Posted by "jonahgao (via GitHub)" <gi...@apache.org>.
jonahgao merged PR #14489:
URL: https://github.com/apache/datafusion/pull/14489
--
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] bug: Fix edge cases in array_slice [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14489:
URL: https://github.com/apache/datafusion/pull/14489#discussion_r1942981686
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -1993,6 +1993,28 @@ select array_slice(arrow_cast(make_array(1, 2, 3, 4, 5), 'LargeList(Int64)'), 2,
----
[2, 3, 4] [h, e]
+# array_slice scalar function #24 (with first negative index larger than len)
+query ??
+select array_slice(make_array(1, 2, 3, 4, 5), -2147483648, 1), list_slice(make_array('h', 'e', 'l', 'l', 'o'), -2147483648, 1);
+----
+[1] [h]
+
+query ??
+select array_slice(arrow_cast(make_array(1, 2, 3, 4, 5), 'LargeList(Int64)'), -9223372036854775808, 1), list_slice(arrow_cast(make_array('h', 'e', 'l', 'l', 'o'), 'LargeList(Utf8)'), -9223372036854775808, 1);
Review Comment:
```sql
D select array_slice([1, 2, 3, 4, 5], -2147483648, 1)
;
┌─────────────────────────────────────────────────────────────┐
│ array_slice(main.list_value(1, 2, 3, 4, 5), -2147483648, 1) │
│ int32[] │
├─────────────────────────────────────────────────────────────┤
│ [1] │
└─────────────────────────────────────────────────────────────┘
```
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -1941,12 +1941,12 @@ select array_slice(arrow_cast(make_array(1, 2, 3, 4, 5), 'LargeList(Int64)'), -4
query ??
select array_slice(make_array(1, 2, 3, 4, 5), -7, -2), array_slice(make_array('h', 'e', 'l', 'l', 'o'), -7, -3);
Review Comment:
Verified this is consisent with DuckDB
```sql
D select array_slice([1, 2, 3, 4, 5], -7, -2)
;
┌─────────────────────────────────────────────────────┐
│ array_slice(main.list_value(1, 2, 3, 4, 5), -7, -2) │
│ int32[] │
├─────────────────────────────────────────────────────┤
│ [1, 2, 3, 4] │
└─────────────────────────────────────────────────────┘
```
D select array_slice(['h', 'e', 'l', 'l', 'o'], -7, -3);
┌───────────────────────────────────────────────────────────────┐
│ array_slice(main.list_value('h', 'e', 'l', 'l', 'o'), -7, -3) │
│ varchar[] │
├───────────────────────────────────────────────────────────────┤
│ [h, e, l] │
└───────────────────────────────────────────────────────────────┘
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -487,7 +487,17 @@ where
// 0 ~ len - 1
let adjusted_zero_index = if index < 0 {
if let Ok(index) = index.try_into() {
- index + len
+ // When index < 0 and -index > length, index is clamped to the beginning of the list.
+ // Otherwise, when index < 0, the index is counted from the end of the list.
+ //
+ // Note, we actually test the contrapositive, index < -length, because negating a
+ // negative will panic if the negative is equal to the smallest representable value
+ // while negating a positive is always safe.
+ if index < (O::zero() - O::one()) * len {
Review Comment:
I played around with it and I could not find anything better than this
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -1993,6 +1993,28 @@ select array_slice(arrow_cast(make_array(1, 2, 3, 4, 5), 'LargeList(Int64)'), 2,
----
[2, 3, 4] [h, e]
+# array_slice scalar function #24 (with first negative index larger than len)
+query ??
+select array_slice(make_array(1, 2, 3, 4, 5), -2147483648, 1), list_slice(make_array('h', 'e', 'l', 'l', 'o'), -2147483648, 1);
+----
+[1] [h]
+
+query ??
+select array_slice(arrow_cast(make_array(1, 2, 3, 4, 5), 'LargeList(Int64)'), -9223372036854775808, 1), list_slice(arrow_cast(make_array('h', 'e', 'l', 'l', 'o'), 'LargeList(Utf8)'), -9223372036854775808, 1);
+----
+[1] [h]
+
+# array_slice scalar function #25 (with negative step and equal indexes)
+query ??
+select array_slice(make_array(1, 2, 3, 4, 5), 2, 2, -1), list_slice(make_array('h', 'e', 'l', 'l', 'o'), 2, 2, -1);
Review Comment:
```sql
D select array_slice([1, 2, 3, 4, 5], 2, 2, -1);
┌───────────────────────────────────────────────────────┐
│ array_slice(main.list_value(1, 2, 3, 4, 5), 2, 2, -1) │
│ int32[] │
├───────────────────────────────────────────────────────┤
│ [2] │
└───────────────────────────────────────────────────────
```
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -1993,6 +1993,28 @@ select array_slice(arrow_cast(make_array(1, 2, 3, 4, 5), 'LargeList(Int64)'), 2,
----
[2, 3, 4] [h, e]
+# array_slice scalar function #24 (with first negative index larger than len)
+query ??
+select array_slice(make_array(1, 2, 3, 4, 5), -2147483648, 1), list_slice(make_array('h', 'e', 'l', 'l', 'o'), -2147483648, 1);
Review Comment:
BTW I know this is consistent with the other tests in this file (so we shouldn't change it in this PR), but DataFusion now supports the `[]` array literal syntax too:
```sql
(venv) andrewlamb@Andrews-MacBook-Pro-2:~/Software/datafusion$ datafusion-cli
DataFusion CLI v45.0.0
> select array_slice([1, 2, 3, 4, 5], -7, -2);
+-------------------------------------------------------------------------------+
| make_array(Int64(1),Int64(2),Int64(3),Int64(4),Int64(5))[Int64(-7):Int64(-2)] |
+-------------------------------------------------------------------------------+
| [] |
+-------------------------------------------------------------------------------+
1 row(s) fetched.
Elapsed 0.049 seconds.
```
--
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] bug: Fix edge cases in array_slice [datafusion]
Posted by "jkosh44 (via GitHub)" <gi...@apache.org>.
jkosh44 commented on PR #14489:
URL: https://github.com/apache/datafusion/pull/14489#issuecomment-2637074517
> I think this PR is nicely implemented, commented and tested 🏆
Thank you!
--
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] bug: Fix edge cases in array_slice [datafusion]
Posted by "jkosh44 (via GitHub)" <gi...@apache.org>.
jkosh44 commented on code in PR #14489:
URL: https://github.com/apache/datafusion/pull/14489#discussion_r1941739956
##########
datafusion/functions-nested/src/extract.rs:
##########
@@ -487,7 +487,17 @@ where
// 0 ~ len - 1
let adjusted_zero_index = if index < 0 {
if let Ok(index) = index.try_into() {
- index + len
+ // When index < 0 and -index > length, index is clamped to the beginning of the list.
+ // Otherwise, when index < 0, the index is counted from the end of the list.
+ //
+ // Note, we actually test the contrapositive, index < -length, because negating a
+ // negative will panic if the negative is equal to the smallest representable value
+ // while negating a positive is always safe.
+ if index < (O::zero() - O::one()) * len {
Review Comment:
If anyone knows a simpler way of negating `len`, please let me know.
--
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