From 321cf6ad7a320f5e8c5365d7f8cc0804c1ff90e3 Mon Sep 17 00:00:00 2001 From: Michael Kolupaev Date: Tue, 5 Nov 2024 00:34:48 +0000 Subject: [PATCH 1/2] Fix negate function monotonicity --- src/Functions/negate.cpp | 25 +++++++++++++++++-- .../03259_negate_key_overflow.reference | 3 +++ .../0_stateless/03259_negate_key_overflow.sql | 14 +++++++++++ 3 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 tests/queries/0_stateless/03259_negate_key_overflow.reference create mode 100644 tests/queries/0_stateless/03259_negate_key_overflow.sql diff --git a/src/Functions/negate.cpp b/src/Functions/negate.cpp index 2c9b461274d..e1f0390f9cb 100644 --- a/src/Functions/negate.cpp +++ b/src/Functions/negate.cpp @@ -32,9 +32,30 @@ using FunctionNegate = FunctionUnaryArithmetic; template <> struct FunctionUnaryArithmeticMonotonicity { static bool has() { return true; } - static IFunction::Monotonicity get(const Field &, const Field &) + static IFunction::Monotonicity get(const Field & left, const Field & right) { - return { .is_monotonic = true, .is_positive = false, .is_strict = true }; + /// negate(UInt64) -> Int64: + /// * monotonically decreases on [0, 2^63] (no overflow), + /// * then jumps up from -2^63 to 2^63-1, then + /// * monotonically decreases on [2^63+1, 2^64-1] (with overflow). + /// Similarly for UInt128 and UInt256. + bool is_monotonic = true; + switch (left.getType()) + { + case Field::Types::UInt64: + is_monotonic = (left.safeGet() > 1ul << 63) == (right.safeGet() > 1ul << 63); + break; + case Field::Types::UInt128: + is_monotonic = (left.safeGet() > UInt128(1) << 127) == (right.safeGet() > UInt128(1) << 127); + break; + case Field::Types::UInt256: + is_monotonic = (left.safeGet() > UInt256(1) << 255) == (right.safeGet() > UInt256(1) << 255); + break; + default: + break; + } + + return { .is_monotonic = is_monotonic, .is_positive = false, .is_strict = true }; } }; diff --git a/tests/queries/0_stateless/03259_negate_key_overflow.reference b/tests/queries/0_stateless/03259_negate_key_overflow.reference new file mode 100644 index 00000000000..bda709ecfb4 --- /dev/null +++ b/tests/queries/0_stateless/03259_negate_key_overflow.reference @@ -0,0 +1,3 @@ +42 +42 +42 diff --git a/tests/queries/0_stateless/03259_negate_key_overflow.sql b/tests/queries/0_stateless/03259_negate_key_overflow.sql new file mode 100644 index 00000000000..eb825615e74 --- /dev/null +++ b/tests/queries/0_stateless/03259_negate_key_overflow.sql @@ -0,0 +1,14 @@ +create table a (x UInt64) engine MergeTree order by x; +insert into a values (12345678901234567890), (42); +select * from a where -x = -42; +drop table a; + +create table a (x UInt128) engine MergeTree order by x; +insert into a values (170141183460469231731687303715884105828), (42); +select * from a where -x = -42; +drop table a; + +create table a (x UInt256) engine MergeTree order by x; +insert into a values (57896044618658097711785492504343953926634992332820282019728792003956564820068), (42); +select * from a where -x = -42; +drop table a; From 539b17c69353f6be5c54b6f7842915778b9c136e Mon Sep 17 00:00:00 2001 From: Michael Kolupaev Date: Sat, 9 Nov 2024 00:00:42 +0000 Subject: [PATCH 2/2] Fix nulls/infinities --- src/Functions/negate.cpp | 48 +++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/src/Functions/negate.cpp b/src/Functions/negate.cpp index e1f0390f9cb..91e64e98c6b 100644 --- a/src/Functions/negate.cpp +++ b/src/Functions/negate.cpp @@ -39,22 +39,40 @@ template <> struct FunctionUnaryArithmeticMonotonicity /// * then jumps up from -2^63 to 2^63-1, then /// * monotonically decreases on [2^63+1, 2^64-1] (with overflow). /// Similarly for UInt128 and UInt256. - bool is_monotonic = true; - switch (left.getType()) - { - case Field::Types::UInt64: - is_monotonic = (left.safeGet() > 1ul << 63) == (right.safeGet() > 1ul << 63); - break; - case Field::Types::UInt128: - is_monotonic = (left.safeGet() > UInt128(1) << 127) == (right.safeGet() > UInt128(1) << 127); - break; - case Field::Types::UInt256: - is_monotonic = (left.safeGet() > UInt256(1) << 255) == (right.safeGet() > UInt256(1) << 255); - break; - default: - break; - } + /// Note: we currently don't handle the corner case -UINT64_MIN == UINT64_MIN, and similar for floats and wide signed ints. + /// (This implementation seems overcomplicated and not very correct, maybe there's a better way to do it, + /// maybe by using the actual IDataType instead of two field types.) + /// We don't know the data type, assume nonmonotonic. + if (left.isNull() && right.isNull()) + return { .is_monotonic = false, .is_positive = false, .is_strict = true }; + + auto which_half_if_unsigned_or_infinity = [](const Field & f, int half_if_null, bool & is_unsigned) -> int + { + is_unsigned = true; + switch (f.getType()) + { + case Field::Types::UInt64: return (f.safeGet() > 1ul << 63) ? +1 : -1; + case Field::Types::UInt128: return (f.safeGet() > UInt128(1) << 127) ? +1 : -1; + case Field::Types::UInt256: return (f.safeGet() > UInt256(1) << 255) ? +1 : -1; + default: break; + } + is_unsigned = false; + if (f.isPositiveInfinity()) + return +1; + if (f.isNegativeInfinity()) + return -1; + if (f.isNull()) + return half_if_null; + return 0; + }; + bool left_is_unsigned, right_is_unsigned; + int left_half = which_half_if_unsigned_or_infinity(left, -1, left_is_unsigned); + int right_half = which_half_if_unsigned_or_infinity(right, +1, right_is_unsigned); + + bool is_monotonic = true; + if (left_is_unsigned || right_is_unsigned) + is_monotonic = left_half * right_half >= 0; /// either both values in the same half or unexpected signed-unsigned mix return { .is_monotonic = is_monotonic, .is_positive = false, .is_strict = true }; } };