smallzhongfeng commented on code in PR #8313:
URL: https://github.com/apache/arrow-datafusion/pull/8313#discussion_r1403544744
##########
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);
let step = step_array.as_ref().map(|arr| arr.value(idx)).unwrap_or(1);
if step == 0 {
return exec_err!("step can't be 0 for function range(start [,
stop, step]");
}
- let value = (start..stop).step_by(step as usize);
- values.extend(value);
+ 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());
+ }
+
Review 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. 🤔️
##########
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);
let step = step_array.as_ref().map(|arr| arr.value(idx)).unwrap_or(1);
if step == 0 {
return exec_err!("step can't be 0 for function range(start [,
stop, step]");
}
- let value = (start..stop).step_by(step as usize);
- values.extend(value);
+ 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());
+ }
+
Review 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. 🤔️
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
