Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect modulo(a, b), a % b behavior for Int fields #20052

Closed
Minisai opened this issue Feb 3, 2021 · 4 comments · Fixed by #20067
Closed

Incorrect modulo(a, b), a % b behavior for Int fields #20052

Minisai opened this issue Feb 3, 2021 · 4 comments · Fixed by #20067
Assignees
Labels
bug Confirmed user-visible misbehaviour in official release st-accepted The issue is in our backlog, ready to take

Comments

@Minisai
Copy link

Minisai commented Feb 3, 2021

ClickHouse server version 20.8.9 revision 54438

Let's imagine some simple table structure:

CREATE TABLE users (
    `user_id` Int32
)

Modulo operator on Int32 field with 150 argument returns negative result. This could have happened due to wrong type casting - result should be casted to Int16 or UInt8 instead of Int8.

SELECT 
    user_id % 150, 
    441746 % 150,
    toTypeName(user_id % 150),
    toTypeName(441746 % 150)
FROM users 
WHERE user_id = 441746
LIMIT 1
┌─modulo(user_id, 150)─┬─modulo(441746, 150)─┬─toTypeName(modulo(user_id, 150))─┬─toTypeName(modulo(441746, 150))─┐
│                -110  │                 146 │ Int8                             │ UInt8                           │
└──────────────────────┴─────────────────────┴──────────────────────────────────┴─────────────────────────────────┘
@Minisai Minisai added the bug Confirmed user-visible misbehaviour in official release label Feb 3, 2021
@hexiaoting
Copy link
Contributor

hexiaoting commented Feb 4, 2021

dell123 :) select toInt32(441746) % 150 as a , toTypeName(a), 441746 % 150 as b, toTypeName(b)

Query id: e0111eab-044c-47da-9684-084bf8d07a01

┌────a─┬─toTypeName(modulo(toInt32(441746), 150))─┬───b─┬─toTypeName(modulo(441746, 150))─┐
│ -110 │ Int8                                     │ 146 │ UInt8                           │
└──────┴──────────────────────────────────────────┴─────┴─────────────────────────────────┘

1 rows in set. Elapsed: 0.003 sec.

dell123 :)

The type of constant 150 is UInt8, and 441746 is UInt32.
when Int % UInt8 the result type is Int8.
when UInt32 % UInt8 the result type is UInt8.

So if you want the same result, maybe you can try:

SELECT
    user_id % toUInt32(150),
    441746 % toUInt32(150),
    toTypeName(user_id % toUInt32(150)),
    toTypeName(441746 % toUInt32(150))
FROM users
WHERE user_id = 441746
LIMIT 1

Query id: f9e59396-751f-46f2-af1e-40a575bf6c08

┌─modulo(user_id, toUInt32(150))─┬─modulo(441746, toUInt32(150))─┬─toTypeName(modulo(user_id, toUInt32(150)))─┬─toTypeName(modulo(441746, toUInt32(150)))─┐
│                            146 │                           146 │ Int32                                      │ UInt32                                    │
└────────────────────────────────┴───────────────────────────────┴────────────────────────────────────────────┴───────────────────────────────────────────┘



SELECT
    user_id % toInt32(150),
    441746 % toInt32(150),
    toTypeName(user_id % toInt32(150)),
    toTypeName(441746 % toInt32(150))
FROM users
WHERE user_id = 441746
LIMIT 1

Query id: d649e691-43ea-41ca-9ee3-46d8cb362a73

┌─modulo(user_id, toInt32(150))─┬─modulo(441746, toInt32(150))─┬─toTypeName(modulo(user_id, toInt32(150)))─┬─toTypeName(modulo(441746, toInt32(150)))─┐
│                           146 │                          146 │ Int32                                     │ Int32                                    │
└───────────────────────────────┴──────────────────────────────┴───────────────────────────────────────────┴──────────────────────────────────────────┘

@zhangjmruc
Copy link
Contributor

A simple analysis:
The incorrect result may happen when the type A is signed and type B is usigned, because the result type of modulo is signed B. In this case, type A is signed Int32, type B is unsigned UInt8, hence the result type is Int8. The value of UInt8 from 128 to 255 is considered as negative for type Int8.

=== code ====
src/DataTypes/NumbeTraits.h
/** Division with remainder you get a number with the same number of bits as in divisor.
*/
template <typename A, typename B> struct ResultOfModulo
{
using Type0 = typename Construct<is_signed_v || is_signed_v, false, sizeof(B)>::Type;
using Type = std::conditional_t<std::is_floating_point_v
|| std::is_floating_point_v, Float64, Type0>;
};

template <> struct Construct<true, false, 1> { using Type = Int8; }; <<<< Maybe we should use Int16?

@alexey-milovidov
Copy link
Member

You are right, we need to use larger signed type to accomodate the result.

@alexey-milovidov
Copy link
Member

Minimal example: SELECT toInt32(-199) % 200.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed user-visible misbehaviour in official release st-accepted The issue is in our backlog, ready to take
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants