Skip to content

Commit 47f75b2

Browse files
committed
fix: extract scalar comparison logic into min_max_scalar function
- Resolved non-local return issue in macro by moving scalar comparison logic to `min_max_scalar(...) -> Result<ScalarValue>`. - Maintained `min_max!` as a thin wrapper for existing call sites. - Updated dictionary error-path tests to directly call `min_max_scalar`, preserving behavior while enhancing testability for error cases.
1 parent 7bd29e1 commit 47f75b2

1 file changed

Lines changed: 27 additions & 15 deletions

File tree

  • datafusion/functions-aggregate-common/src

datafusion/functions-aggregate-common/src/min_max.rs

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -145,9 +145,9 @@ macro_rules! min_max_generic {
145145
// Dictionary scalars participate by comparing their inner logical values.
146146
// When both inputs are dictionaries, matching key types are preserved in the
147147
// result; differing key types remain an unexpected invariant violation.
148-
macro_rules! min_max {
148+
macro_rules! min_max_scalar_impl {
149149
($VALUE:expr, $DELTA:expr, $OP:ident) => {{
150-
Ok(match ($VALUE, $DELTA) {
150+
match ($VALUE, $DELTA) {
151151
(ScalarValue::Null, ScalarValue::Null) => ScalarValue::Null,
152152
(
153153
lhs @ ScalarValue::Decimal32(lhsv, lhsp, lhss),
@@ -456,7 +456,25 @@ macro_rules! min_max {
456456
e
457457
)
458458
}
459-
})
459+
}
460+
}};
461+
}
462+
463+
fn min_max_scalar(
464+
lhs: &ScalarValue,
465+
rhs: &ScalarValue,
466+
ordering: Ordering,
467+
) -> Result<ScalarValue> {
468+
match ordering {
469+
Ordering::Greater => Ok(min_max_scalar_impl!(lhs, rhs, min)),
470+
Ordering::Less => Ok(min_max_scalar_impl!(lhs, rhs, max)),
471+
Ordering::Equal => unreachable!("min/max comparisons do not use equal ordering"),
472+
}
473+
}
474+
475+
macro_rules! min_max {
476+
($VALUE:expr, $DELTA:expr, $OP:ident) => {{
477+
min_max_scalar($VALUE, $DELTA, choose_min_max!($OP))
460478
}};
461479
}
462480

@@ -497,11 +515,7 @@ fn dictionary_inner_scalar_min_max(
497515
rhs: &ScalarValue,
498516
ordering: Ordering,
499517
) -> Result<ScalarValue> {
500-
match ordering {
501-
Ordering::Greater => min_max!(lhs, rhs, min),
502-
Ordering::Less => min_max!(lhs, rhs, max),
503-
Ordering::Equal => unreachable!("min/max comparisons do not use equal ordering"),
504-
}
518+
min_max_scalar(lhs, rhs, ordering)
505519
}
506520

507521
/// An accumulator to compute the maximum value
@@ -916,9 +930,7 @@ mod tests {
916930
);
917931
let scalar = ScalarValue::Float32(Some(2.0));
918932

919-
let result: Result<ScalarValue, DataFusionError> =
920-
min_max!(&dictionary, &scalar, max);
921-
let result = result?;
933+
let result = min_max_scalar(&dictionary, &scalar, Ordering::Less)?;
922934

923935
assert_eq!(result, ScalarValue::Float32(Some(2.0)));
924936
Ok(())
@@ -935,8 +947,7 @@ mod tests {
935947
Box::new(ScalarValue::Float32(Some(2.0))),
936948
);
937949

938-
let result: Result<ScalarValue, DataFusionError> = min_max!(&lhs, &rhs, max);
939-
let result = result?;
950+
let result = min_max_scalar(&lhs, &rhs, Ordering::Less)?;
940951

941952
assert_eq!(
942953
result,
@@ -959,7 +970,8 @@ mod tests {
959970
Box::new(ScalarValue::Float32(Some(2.0))),
960971
);
961972

962-
let error: DataFusionError = min_max!(&lhs, &rhs, max).unwrap_err();
973+
let error: DataFusionError =
974+
min_max_scalar(&lhs, &rhs, Ordering::Less).unwrap_err();
963975

964976
assert!(
965977
error
@@ -978,7 +990,7 @@ mod tests {
978990
let scalar = ScalarValue::Int32(Some(2));
979991

980992
let error: DataFusionError =
981-
min_max!(&dictionary, &scalar, max).unwrap_err();
993+
min_max_scalar(&dictionary, &scalar, Ordering::Less).unwrap_err();
982994

983995
assert!(
984996
error

0 commit comments

Comments
 (0)