Add support for function `mod` #2402

riteme · 2021-07-14T09:14:44Z

What problem does this PR solve?

Issue Number: close #2318

Problem Summary: enable pushing function mod down to TiFlash.

What is changed and how it works?

Proposal:

What's Changed:

Decimal.h: compatible with TiDB.
DataTypeDecimal.h: add a helper function to get decimal precision in unit test.
DAGUtils.cpp: add mappings from TiPB codes to scalar function name.
FunctionArithmetic.h: update implementation of FunctionModulo.
NumberTraits.h: fix modulo result type deduction.
types.h: add actual_type, which returns correct size of boost multiprecision integer.
mod.test, mod_extra.test: the integration test.
gtest_arithmetic_functions.cpp: the unit test.

Related changes

PR to update pingcap/docs/pingcap/docs-cn:
Need to cherry-pick to the release branch:

Check List

Tests

Unit test
Integration test

Release note

Support pushing down MOD(N, M) to TiFlash.

dbms/src/Functions/FunctionsArithmetic.h

dbms/src/Flash/Coprocessor/DAGUtils.cpp

riteme · 2021-07-15T09:23:21Z

/run-all-tests

riteme · 2021-07-16T11:44:51Z

/run-all-tests

riteme · 2021-07-19T04:49:40Z

/run-all-tests

dbms/src/DataTypes/NumberTraits.h

dbms/src/Functions/FunctionsArithmetic.h

fuzhe1989 · 2021-07-16T04:04:30Z

dbms/src/Functions/FunctionsArithmetic.h

+        }
+        else
+        {
+            return static_cast<Result>(a) % static_cast<Result>(b);


When both A and B are not decimal, the Result shouldn't be decimal. Suggest only leaving a static_assert(!IsDecimal<Result>) here.

dbms/src/Functions/FunctionsArithmetic.h

fuzhe1989 · 2021-07-19T05:33:20Z

dbms/src/Functions/FunctionsArithmetic.h

+        }
+        else  // both A and B are integrals.
+        {
+            using UnsignedResultType = std::make_unsigned_t<Result>;


suggest using tics version of make_unsigned_t and is_signed_v.

dbms/src/Functions/FunctionsArithmetic.h

fuzhe1989 · 2021-07-19T05:45:20Z

dbms/src/Functions/FunctionsArithmetic.h

+                x = static_cast<UnsignedResultType>(a);
+
+            if (std::is_signed_v<B> && b < 0)
+                y = static_cast<UnsignedResultType>(-b);


dbms/src/Functions/FunctionsArithmetic.h

tests/fullstack-test/expr/mod.test

riteme · 2021-07-19T13:01:28Z

/run-all-tests

riteme · 2021-07-21T08:18:06Z

/run-all-tests

fuzhe1989 · 2021-07-21T08:41:36Z

dbms/src/DataTypes/NumberTraits.h

+ * TODO:
+ * * add 128-bit and 256-bit integers.
+ * * change Float32 to Float64.
+ *   * all floating-point arithmetics should evaluate to Float64.


Suggested change

* * all floating-point arithmetics should evaluate to Float64.

* * all floating-point arithmetics should be evaluated to Float64.

Past tense may not be needed. Here are some examples: https://linggle.com/?q=evaluate+to

Guess they have different subjects. Anyway either style is okay.

dbms/src/DataTypes/NumberTraits.h

dbms/src/Functions/FunctionsArithmetic.h

fuzhe1989 · 2021-07-21T12:56:52Z

dbms/src/Functions/FunctionsArithmetic.h

+            auto & a_nullmap_data = a_nullmap->getData();
+            auto & b_nullmap_data = b_nullmap->getData();
+            for (size_t i = 0; i < size; ++i)
+                res_null[i] = a_nullmap_data[i] || b_nullmap_data[i];


https://godbolt.org/z/Kj7chc3xo seems | can generate simpler code than ||.

Anyway I found compiler won't automatically vectorize this for-loop.

dbms/src/Functions/FunctionsArithmetic.h

fuzhe1989 · 2021-07-21T13:00:01Z

dbms/src/Functions/FunctionsArithmetic.h

+        }
+        else if constexpr (is_modulo)
+        {
+            if (scale_a != 1)


Got it, so would you like to paste your explanation into the code?

Co-authored-by: Fu Zhe <fuzhe1989@gmail.com>

fuzhe1989 · 2021-07-22T02:05:02Z

/run-all-tests

ti-srebot · 2021-07-22T03:28:35Z

@fuzhe1989, Thanks for your review. The bot only counts LGTMs from Reviewers and higher roles, but you're still welcome to leave your comments. See the corresponding SIG page for more information. Related SIG: tiflash(slack).

fuzhe1989 · 2021-07-22T03:28:59Z

/merge

ti-srebot · 2021-07-22T03:29:21Z

/run-all-tests

zanmato1984 · 2021-08-04T05:35:36Z

/run-cherry-picker

ti-srebot · 2021-08-04T05:42:58Z

cherry pick to release-5.1 in PR #2560

ti-srebot · 2021-08-04T05:50:01Z

cherry pick to release-5.0 in PR #2561

riteme added 3 commits July 14, 2021 11:53

Add fullstack test for function mod.

7fc41f9

Add TiPB code mappings for mod.

b2209ae

mod: basic implementation.

04870a0

leiysky reviewed Jul 14, 2021

View reviewed changes

dbms/src/Functions/FunctionsArithmetic.h Show resolved Hide resolved

leiysky reviewed Jul 14, 2021

View reviewed changes

dbms/src/Flash/Coprocessor/DAGUtils.cpp Outdated Show resolved Hide resolved

riteme changed the title ~~[DNM] Add support for function mod~~ [DNM][WIP] Add support for function mod Jul 14, 2021

riteme added 3 commits July 15, 2021 09:58

remove useless line.

89ec101

mod: add Decimal and Real.

11d08a8

style: keep original.

d6fda33

riteme changed the title ~~[DNM][WIP] Add support for function mod~~ [DNM] Add support for function mod Jul 15, 2021

riteme and others added 4 commits July 16, 2021 17:25

fix: unsigned % signed and signed % unsigned.

d10e49f

fix: result sign.

3926353

test: complete unit test.

e7bcd1c

Merge branch 'master' into function-mod

5a31f39

riteme changed the title ~~[DNM] Add support for function mod~~ Add support for function mod Jul 16, 2021

riteme added 3 commits July 19, 2021 12:21

remove dead code.

f67474f

Merge remote-tracking branch 'origin/master' into function-mod

988415c

test: rename NativeType to FieldType.

971fc9d

fuzhe1989 reviewed Jul 19, 2021

View reviewed changes

fix: review comments.

fb64aa8

fuzhe1989 reviewed Jul 19, 2021

View reviewed changes

tests/fullstack-test/expr/mod.test Show resolved Hide resolved

tests/fullstack-test/expr/mod.test Show resolved Hide resolved

riteme added 3 commits July 19, 2021 17:23

fix: int precison.

28ef9ce

fix: Decimal256 & Decimal512.

29a5268

fix: static_cast.

69ebf00

test: Decimal128 & Decimal256.

a8afdbf

riteme and others added 4 commits July 21, 2021 15:50

test: unit test ModuloExtra.

3200f1c

style: remove blank line.

0393ac3

style: fix typo.

3b62d92

Merge branch 'master' into function-mod

d68c12c

fuzhe1989 reviewed Jul 21, 2021

View reviewed changes

riteme and others added 2 commits July 22, 2021 09:43

update dbms/src/DataTypes/NumberTraits.h

3765b28

Co-authored-by: Fu Zhe <fuzhe1989@gmail.com>

update dbms/src/Functions/FunctionsArithmetic.h

7f28399

Co-authored-by: Fu Zhe <fuzhe1989@gmail.com>

riteme and others added 2 commits July 22, 2021 10:47

Merge branch 'master' into function-mod

a8f56c8

style: update TODO on struct Construct.

d96057b

fuzhe1989 approved these changes Jul 22, 2021

View reviewed changes

ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 22, 2021

riteme mentioned this pull request Jul 22, 2021

Refactor FunctionBinaryArithmetic to make it more readable #2419

Closed

ti-srebot merged commit 685880a into pingcap:master Jul 22, 2021

riteme mentioned this pull request Jul 22, 2021

Fix function mod decimal scale overflow #2462

Merged

zanmato1984 added needs-cherry-pick-release-5.0 PR which needs to be cherry-picked to release-5.0 needs-cherry-pick-release-5.1 PR which needs to be cherry-picked to release-5.1 labels Aug 4, 2021

zanmato1984 added the type/enhancement Issue or PR for enhancement label Aug 4, 2021

ti-srebot mentioned this pull request Aug 4, 2021

Add support for function mod (#2402) #2560

Closed

7 tasks

ti-srebot mentioned this pull request Aug 4, 2021

Add support for function mod (#2402) #2561

Closed

7 tasks

This was referenced Aug 6, 2021

Fix function mod decimal scale overflow (#2462) #2608

Closed

Fix function mod decimal scale overflow (#2462) #2609

Closed

guo-shaoge mentioned this pull request Feb 9, 2022

Optimize overflow check when cast to decimal #3912

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for function `mod` #2402

Add support for function `mod` #2402

riteme commented Jul 14, 2021 •

edited

Loading

riteme commented Jul 15, 2021

riteme commented Jul 16, 2021

riteme commented Jul 19, 2021

fuzhe1989 Jul 16, 2021

fuzhe1989 Jul 19, 2021

fuzhe1989 Jul 19, 2021

riteme commented Jul 19, 2021

riteme commented Jul 21, 2021

fuzhe1989 Jul 21, 2021

riteme Jul 22, 2021

fuzhe1989 Jul 22, 2021

fuzhe1989 Jul 21, 2021

fuzhe1989 Jul 21, 2021

fuzhe1989 commented Jul 22, 2021

ti-srebot commented Jul 22, 2021

fuzhe1989 commented Jul 22, 2021

ti-srebot commented Jul 22, 2021

zanmato1984 commented Aug 4, 2021

ti-srebot commented Aug 4, 2021

ti-srebot commented Aug 4, 2021

	* * all floating-point arithmetics should evaluate to Float64.
	* * all floating-point arithmetics should be evaluated to Float64.

Add support for function mod #2402

Add support for function mod #2402

Conversation

riteme commented Jul 14, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

riteme commented Jul 15, 2021

riteme commented Jul 16, 2021

riteme commented Jul 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riteme commented Jul 19, 2021

riteme commented Jul 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuzhe1989 commented Jul 22, 2021

ti-srebot commented Jul 22, 2021

fuzhe1989 commented Jul 22, 2021

ti-srebot commented Jul 22, 2021

zanmato1984 commented Aug 4, 2021

ti-srebot commented Aug 4, 2021

ti-srebot commented Aug 4, 2021

Add support for function `mod` #2402

Add support for function `mod` #2402

riteme commented Jul 14, 2021 •

edited

Loading