Skip to content

Commit

Permalink
Compute gcd with u64 instead of i64 because of overflows (apache#11036)
Browse files Browse the repository at this point in the history
* compute gcd with unsigned ints

* add test for the i64::MAX cases

* move unsigned_abs below zero test to remove unnecessary casts

* add slt test for gcd on max values instead of unit tests
  • Loading branch information
LorrensP-2158466 authored and xinlifoobar committed Jun 22, 2024
1 parent b0d0f6c commit 41683be
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 8 deletions.
17 changes: 9 additions & 8 deletions datafusion/functions/src/math/gcd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,16 +88,16 @@ fn gcd(args: &[ArrayRef]) -> Result<ArrayRef> {

/// Computes greatest common divisor using Binary GCD algorithm.
pub fn compute_gcd(x: i64, y: i64) -> i64 {
let mut a = x.wrapping_abs();
let mut b = y.wrapping_abs();

if a == 0 {
return b;
if x == 0 {
return y;
}
if b == 0 {
return a;
if y == 0 {
return x;
}

let mut a = x.unsigned_abs();
let mut b = y.unsigned_abs();

let shift = (a | b).trailing_zeros();
a >>= shift;
b >>= shift;
Expand All @@ -112,7 +112,8 @@ pub fn compute_gcd(x: i64, y: i64) -> i64 {
b -= a;

if b == 0 {
return a << shift;
// because the input values are i64, casting this back to i64 is safe
return (a << shift) as i64;
}
}
}
Expand Down
10 changes: 10 additions & 0 deletions datafusion/sqllogictest/test_files/scalar.slt
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,16 @@ select gcd(null, null);
----
NULL

# scalar maxes and/or negative 1
query III rowsort
select
gcd(9223372036854775807, -9223372036854775808), -- i64::MIN, i64::MAX
-- wait till fix, cause it fails gcd(-9223372036854775808, -9223372036854775808), -- -i64::MIN, i64::MIN
gcd(9223372036854775807, -1), -- i64::MAX, -1
gcd(-9223372036854775808, -1); -- i64::MIN, -1
----
1 1 1

# gcd with columns
query III rowsort
select gcd(a, b), gcd(c, d), gcd(e, f) from signed_integers;
Expand Down

0 comments on commit 41683be

Please sign in to comment.