fix: wrong result of range function #8313

smallzhongfeng · 2023-11-23T09:31:31Z

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_by method under iterator, because step_by stipulates that the parameter type is usize

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Veeupup · 2023-11-23T14:34:44Z

thank you for fixing it!

Weijun-H

Thanks @smallzhongfeng 👍

Weijun-H · 2023-11-23T14:59:12Z

datafusion/physical-expr/src/array_expressions.rs

+        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());
+        }
+


Suggested change

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));

}

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. 🤔️

Yes, and I think we should add a test case for it 👍

OK, I added ut for this

Would you mind putting the test in sqllogoictest?

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.

Weijun-H · 2023-11-23T14:59:27Z

datafusion/physical-expr/src/array_expressions.rs

@@ -741,13 +741,22 @@ pub fn gen_range(args: &[ArrayRef]) -> Result<ArrayRef> {
    let mut offsets = vec![0];
    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);


Suggested change

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);

Weijun-H · 2023-11-24T09:26:33Z

datafusion/sqllogictest/test_files/array.slt

@@ -2752,7 +2752,7 @@ select range(5),
       range(1, -5, 1)
 ;


Suggested change

;

range(1, -5, -1)

;

move the test to the sqllogistest, overall LGTM 👍

jayzhan211 · 2023-11-25T03:30:19Z

datafusion/physical-expr/src/array_expressions.rs

@@ -2514,6 +2520,67 @@ mod tests {
            .is_null(0));
    }

+    #[test]
+    fn test_array_range() {


remove test here

@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

I endorse this proposal as it promises to enhance convenience in directing individuals to incorporate new tests, thereby minimizing potential misunderstandings.

@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

Thank you @smallzhongfeng and @Weijun-H and @jayzhan211 for the reviews

alamb · 2023-11-26T12:04:34Z

datafusion/physical-expr/src/array_expressions.rs

@@ -2514,6 +2520,67 @@ mod tests {
            .is_null(0));
    }

+    #[test]
+    fn test_array_range() {


@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

alamb · 2023-11-27T22:34:44Z

Thank you everyone!

zhongjingxiong added 2 commits November 23, 2023 17:12

fix: wrong result of range function

fc6e0af

fix test

f7b32c8

github-actions bot added physical-expr Physical Expressions sqllogictest labels Nov 23, 2023

Weijun-H reviewed Nov 23, 2023

View reviewed changes

add ut

aedd68a

Weijun-H reviewed Nov 24, 2023

View reviewed changes

zhongjingxiong added 3 commits November 24, 2023 17:38

add ut

7bc78c6

nit

d573c00

nit

63c8efa

smallzhongfeng force-pushed the fix-range-step-negative-number branch from 39f5185 to 63c8efa Compare November 24, 2023 12:52

jayzhan211 reviewed Nov 25, 2023

View reviewed changes

alamb approved these changes Nov 26, 2023

View reviewed changes

jayzhan211 mentioned this pull request Nov 27, 2023

Port tests in array_expression.rs to sqllogictest #8324

Closed

13 tasks

alamb merged commit 9dfb224 into apache:main Nov 27, 2023
22 checks passed

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: wrong result of range function #8313

fix: wrong result of range function #8313

smallzhongfeng commented Nov 23, 2023

Veeupup commented Nov 23, 2023

Weijun-H left a comment

Weijun-H Nov 23, 2023

smallzhongfeng Nov 23, 2023

Weijun-H Nov 23, 2023

smallzhongfeng Nov 24, 2023

Weijun-H Nov 24, 2023

smallzhongfeng Nov 24, 2023

Weijun-H Nov 23, 2023

Weijun-H Nov 24, 2023

Weijun-H Nov 24, 2023

smallzhongfeng Nov 24, 2023

jayzhan211 Nov 25, 2023

alamb Nov 26, 2023

Weijun-H Nov 26, 2023

jayzhan211 Nov 27, 2023

alamb left a comment

alamb Nov 26, 2023

alamb commented Nov 27, 2023

	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);

fix: wrong result of range function #8313

fix: wrong result of range function #8313

Conversation

smallzhongfeng commented Nov 23, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Veeupup commented Nov 23, 2023

Weijun-H left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Nov 27, 2023