feat: support stride in array_slice, change indexes to be1 based
#8829
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
logical-expr
Weijun-H
changed the title
stride in array_slice
jayzhan211
reviewed
Jan 11, 2024
jayzhan211
reviewed
Jan 11, 2024
|
|
||
| query error Execution error: array_slice got invalid stride: 0, it cannot be 0 | ||
| select array_slice(make_array(1, 2, 3, 4, 5), 1, 5, 0), array_slice(make_array('h', 'e', 'l', 'l', 'o'), 1, 5, 0); | ||
|
|
There was a problem hiding this comment.
SELECT([1, 2, 3, 4, 5])[4:2:-2]; are also needed. And other examples in duckdb, we should consider them too.
There was a problem hiding this comment.
Yes, I think so. And we could do it in ticket #8830 .
Weijun-H
force-pushed
the
feat-array-slice-stride
branch
from
6b56383 to
d41c3a2
Compare
comphead
reviewed
Jan 12, 2024
| @@ -680,6 +689,16 @@ where | |||
| let end = offset_window[1]; | |||
| let len = end - start; | |||
|
|
|||
| let stride = if let Some(stride) = stride { | |||
There was a problem hiding this comment.
this construction most likely can be replaced by .map/.and_then ?
comphead
reviewed
Jan 12, 2024
| (start + to + O::usize_as(1)).to_usize().unwrap(), | ||
| ); | ||
| offsets.push(offsets[row_index] + (to - from + O::usize_as(1))); | ||
| if let Some(stride) = stride { |
There was a problem hiding this comment.
this check may be earlier?
jayzhan211
reviewed
Jan 13, 2024
datafusion/expr/src/expr_fn.rs
Outdated
| @@ -731,7 +731,7 @@ scalar_expr!( | |||
| scalar_expr!( | |||
| ArraySlice, | |||
| array_slice, | |||
| array offset length, | |||
| array offset length stride, | |||
There was a problem hiding this comment.
It seems offset length is incorrect, it should be begin end
Weijun-H
changed the title
stride in array_slicestride in array_slice
Weijun-H
force-pushed
the
feat-array-slice-stride
branch
from
d11a0cc to
977f2e1
Compare
|
stalled until #8847 fixed |
comphead
reviewed
Jan 13, 2024
| @@ -700,17 +709,67 @@ where | |||
| }; | |||
|
|
|||
| if let (Some(from), Some(to)) = (from_index, to_index) { | |||
| let stride = stride.map(|s| s.value(row_index)); | |||
Marking as draft to make it clear this PR is not waiting on review |
Weijun-H
force-pushed
the
feat-array-slice-stride
branch
from
977f2e1 to
297ef22
Compare
9 tasks
alamb
changed the title
stride in array_slicestride in array_slice, change indexes to be 1 based
alamb
changed the title
stride in array_slice, change indexes to be 1 basedstride in array_slice, change indexes to be0 based
alamb
changed the title
stride in array_slice, change indexes to be0 basedstride in array_slice, change indexes to be1 based
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #8784
Rationale for this change
What changes are included in this PR?
support array_slice with stride argument
Are these changes tested?
Are there any user-facing changes?