fix: wrong result of range function#8313
Conversation
github-actions
bot
added
physical-expr
|
thank you for fixing it! |
| let value; | ||
| if step < 0 { | ||
| while stop < start { | ||
| values.push(start); | ||
| start += step; | ||
| } | ||
| } else { | ||
| value = (start..stop).step_by(step as usize); | ||
| values.extend(value.clone()); | ||
| } | ||
|
|
There was a problem hiding this comment.
| let value; | |
| if step < 0 { | |
| while stop < start { | |
| values.push(start); | |
| start += step; | |
| } | |
| } else { | |
| value = (start..stop).step_by(step as usize); | |
| values.extend(value.clone()); | |
| } | |
| if step < 0 { | |
| // Decreasing range | |
| values.extend((stop..start).rev().step_by((-step) as usize)); | |
| } else { | |
| // Increasing range | |
| values.extend((start..stop).step_by(step as usize)); | |
| } |
There was a problem hiding this comment.
Emm, this is good advice, but it should be
values.extend((stop+1..start+1).rev().step_by((-step) as usize));
Because the stop of the range is not taken. 🤔️
There was a problem hiding this comment.
Yes, and I think we should add a test case for it 👍
There was a problem hiding this comment.
OK, I added ut for this
There was a problem hiding this comment.
Would you mind putting the test in sqllogoictest?
There was a problem hiding this comment.
https://github.com/apache/arrow-datafusion/blob/b648d4e22e82989c65523e62312e1995a1543888/datafusion/sqllogictest/test_files/array.slt#L2748C1-L2752C23
I think there is currently this test, but the results were wrong before, and I have corrected them.
| for (idx, stop) in stop_array.iter().enumerate() { | ||
| let stop = stop.unwrap_or(0); | ||
| let start = start_array.as_ref().map(|arr| arr.value(idx)).unwrap_or(0); | ||
| let mut start = start_array.as_ref().map(|arr| arr.value(idx)).unwrap_or(0); |
There was a problem hiding this comment.
| let mut start = start_array.as_ref().map(|arr| arr.value(idx)).unwrap_or(0); | |
| let start = start_array.as_ref().map(|arr| arr.value(idx)).unwrap_or(0); |
| @@ -2752,7 +2752,7 @@ select range(5), | |||
| range(1, -5, 1) | |||
| ; | |||
There was a problem hiding this comment.
| ; | |
| range(1, -5, -1) | |
| ; |
There was a problem hiding this comment.
move the test to the sqllogistest, overall LGTM 👍
smallzhongfeng
force-pushed
the
fix-range-step-negative-number
branch
from
39f5185 to
63c8efa
Compare
| } | ||
|
|
||
| #[test] | ||
| fn test_array_range() { |
There was a problem hiding this comment.
@jayzhan211 and @Weijun-H I think what is happening is that people follow the model of the existing code -- so as long as there are tests in array_expressions (rather than .slt) that is what people will model.
What do you think about migrating (as part of another PR) all the existing tests to sqllogictests -- this would likely result in all subsequent PRs following the sqllogictests model
There was a problem hiding this comment.
I endorse this proposal as it promises to enhance convenience in directing individuals to incorporate new tests, thereby minimizing potential misunderstandings.
There was a problem hiding this comment.
@alamb @Weijun-H
There is one problem actually, function like align_array_dimensions we have a unit test for it but it is not possible to port to slt file. We should either not test it directly but rely on array function test that call it or port it to datafusion::common::utils, and test it there. I think the latter is better so we can check the coverage more easily.
btw, should we introduce array_utils in common. I think it will grow larger and larger quickly.
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @smallzhongfeng and @Weijun-H and @jayzhan211 for the reviews
| } | ||
|
|
||
| #[test] | ||
| fn test_array_range() { |
There was a problem hiding this comment.
@jayzhan211 and @Weijun-H I think what is happening is that people follow the model of the existing code -- so as long as there are tests in array_expressions (rather than .slt) that is what people will model.
What do you think about migrating (as part of another PR) all the existing tests to sqllogictests -- this would likely result in all subsequent PRs following the sqllogictests model
|
Thank you everyone! |
5 participants
Which issue does this PR close?
Closes #8311.
Rationale for this change
When step is a negative number, the decrement calculation is performed from front to back instead of using the
step_bymethod under iterator, becausestep_bystipulates that the parameter type is usizeWhat changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?