Push down `ROUND(x)` on decimal types by riteme · Pull Request #2492 · pingcap/tiflash

riteme · 2021-07-26T04:25:51Z

What problem does this PR solve?

Issue Number: close #2426, part of #2550

Problem Summary: enable ROUND(x) on TiFlash.

What is changed and how it works?

What's Changed:

IDataType.h , DataTypesNumber.h: add isFloatingPoint.
DAGExpressionAnalyzer.cpp: construct tidbRound from tidbRoundWithFrac.
DAGUtils.cpp: map TiPB codes to function names.
FunctionsArithmetic.h, toUnsigned.h: move out toUnsigned for reusing in FunctionsRound.h.
FunctionsRound.cpp: register new function.
FunctionsRound.h: the implementation.

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 ROUND(X) to TiFlash.

riteme · 2021-08-03T09:30:23Z

/run-all-tests

dbms/src/Functions/toUnsigned.h

tests/fullstack-test/expr/round.test

fuzhe1989 · 2021-08-03T13:23:44Z

dbms/src/Flash/Coprocessor/DAGExpressionAnalyzer.cpp

+    if (expr.children_size() != 1)
+        throw TiFlashException("Invalid arguments of ROUND function", Errors::Coprocessor::BadRequest);
+
+    Names argument_names;


nit: you can construct argument_names after const_zero_arg_name is ready.

dbms/src/Functions/FunctionsRound.h

fuzhe1989 · 2021-08-03T13:59:19Z

dbms/src/Functions/FunctionsRound.h

+        block.getByPosition(result).column = std::move(result_column);
+    }
+
+    bool checkInputType(const DataTypePtr & input_type, const DataTypePtr & return_type, const DataTypePtr & frac_type,


there's a mismatch in the parameter orders: for data type the order is [input, return, frac] while for column it is [input, frac, result]

Some combinations of InputType and ReturnType are invalid. I want to stop the recursion as soon as possible to reduce compilation time.

dbms/src/Functions/FunctionsRound.h

fuzhe1989 · 2021-08-03T14:08:52Z

dbms/src/Functions/FunctionsRound.h

+        using ResultColumn = std::conditional_t<IsDecimal<ReturnType>, ColumnDecimal<ReturnType>, ColumnVector<ReturnType>>;
+
+        // TODO: RoundWithFrac
+        EXPECT(!input_column->isColumnConst());


guess I'm reviewing an unfinished pr...

Yeah...It's unfinished because the code framework is prepared for round(x, d).

fuzhe1989 · 2021-08-03T14:10:46Z

dbms/src/Functions/FunctionsRound.h

+        auto input_scale = getDecimalScale(*input_type, 0);
+        auto result_scale = getDecimalScale(*return_type, 0);
+
+        if (!checkInputType(input_type, return_type, frac_type, input_column, frac_column, result_column, input_scale, result_scale))


check sounds like a readonly function but it isn't.

a boolean return value can't tell us anything, we should throw exception from where the check fails.

Any suggestion on naming? What about checkInputTypeAndApply?

I will rewrite the code that throws an exception in each checkXXX function.

riteme · 2021-08-04T11:37:04Z

dbms/src/Flash/Coprocessor/DAGUtils.cpp

+    {tipb::ScalarFuncSig::RoundReal, "tidbRound"},
+    {tipb::ScalarFuncSig::RoundInt, "tidbRound"},
+    {tipb::ScalarFuncSig::RoundDec, "tidbRound"},


Original implementation of round from Clickhouse employed some SIMD computations, but tidbRoundWithFrac currently doesn't make use of them at all.

Maybe reset RoundReal and RoundInt to "round" in this PR for better performance?

riteme · 2021-08-04T11:44:14Z

/run-all-tests

riteme · 2021-08-04T14:49:51Z

/run-all-tests

riteme · 2021-08-05T06:20:14Z

/run-all-tests

dbms/src/Functions/tests/CMakeLists.txt

fuzhe1989 · 2021-08-06T05:56:48Z

dbms/src/Functions/FunctionsRound.h

+// for DEBUG only, will be removed after RoundWithFrac is implemented.
+#define EXPECT(expr) \
+    if (!(expr))     \
+        throw TiFlashException(fmt::format("\"{}\" failed", #expr), Errors::Coprocessor::Internal);


I'm not sure it's good to put expr into error message. Suggest to provide readable error message since it may be seen by users.

I'm going to replace them with assert so that they don't abort in Release mode.

dbms/src/Functions/FunctionsRound.h

fuzhe1989 · 2021-08-06T07:06:02Z

dbms/src/Functions/FunctionsRound.h

+        }
+
+        // in MySQL, frac is clamped to 30, which is identical to decimal_max_scale.
+        if (unsigned_frac > decimal_max_scale)


the result won't be clamped if the field is signed, is it expected?

It's indeed a bug.

dbms/src/Functions/FunctionsRound.h

…-round

fuzhe1989 · 2021-08-06T07:40:29Z

dbms/src/Functions/FunctionsRound.h

    {
        // TODO: RoundWithFrac.
-        EXPECT(frac == 0);
+        assert(frac == 0);


then how will you prevent round(x, frac) from being pushed down?

Only tidbRound is mapped in DAGUtils.cpp.

dbms/src/Functions/FunctionsRound.h

fuzhe1989

rest LGTM

fuzhe1989 · 2021-08-06T08:02:04Z

dbms/src/Functions/FunctionsRound.h

+{
+    static_assert(is_integer_v<InputType>);
+
+    static constexpr InputType eval(const InputType & input, FracType frac)


here [[maybe_unused]]

fuzhe1989 · 2021-08-06T08:02:30Z

dbms/src/Functions/FunctionsRound.h

+    using UnsignedNativeType = make_unsigned_t<typename InputType::NativeType>;
+    using Pow = ConstPowOf10<UnsignedNativeType, maxDecimalPrecision<InputType>()>;
+
+    static constexpr OutputType eval(const InputType & input, FracType frac, ScaleType input_scale)


here [[maybe_unused]]

ti-srebot · 2021-08-06T08:02:49Z

@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).

ti-srebot · 2021-08-06T08:02:49Z

@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).

riteme · 2021-08-06T08:16:30Z

/merge

ti-srebot · 2021-08-06T08:16:57Z

/run-all-tests

ti-srebot · 2021-08-06T09:03:30Z

cherry pick to release-5.0 in PR #2610

ti-srebot · 2021-08-06T09:05:41Z

cherry pick to release-5.1 in PR #2611

riteme changed the title ~~[DNM] Push down ROUND on decimal types~~ [DNM] Push down ROUND(x) on decimal types Aug 3, 2021

riteme changed the title ~~[DNM] Push down ROUND(x) on decimal types~~ [test] Push down ROUND(x) on decimal types Aug 3, 2021

feat: implement round(x).

60a2120

riteme self-assigned this Aug 3, 2021

riteme requested a review from fuzhe1989 August 3, 2021 07:48

test: integration test.

3bbc331

fuzhe1989 reviewed Aug 3, 2021

View reviewed changes

refactor: fix review.

6c8070c

riteme added component/expression type/enhancement The issue or PR belongs to an enhancement. labels Aug 4, 2021

riteme added 2 commits August 4, 2021 19:31

fix: dispatch fails.

51e7379

test: unit test.

097e113

riteme commented Aug 4, 2021

View reviewed changes

style: remove blank line.

1decc46

riteme changed the title ~~[test] Push down ROUND(x) on decimal types~~ Push down ROUND(x) on decimal types Aug 4, 2021

Merge branch 'master' into function-round

de47110

refactor: safeUnsignedAbs -> toSafeUnsigned.

fee29ed

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 5, 2021

Merge branch 'master' into function-round

cbe78d6

riteme requested a review from fuzhe1989 August 5, 2021 07:16

fuzhe1989 reviewed Aug 6, 2021

View reviewed changes

riteme and others added 4 commits August 6, 2021 15:26

fix: review.

210bc74

Merge branch 'master' into function-round

478bbb8

refactor: EXPECT -> assert.

6392025

Merge branch 'function-round' of github.com:riteme/tics into function…

af649b2

…-round

fuzhe1989 reviewed Aug 6, 2021

View reviewed changes

riteme and others added 2 commits August 6, 2021 15:57

fix: review.

5af43c1

Merge branch 'master' into function-round

45f811f

fuzhe1989 approved these changes Aug 6, 2021

View reviewed changes

style: maybe_unused.

01327c4

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

ti-srebot merged commit 4997809 into pingcap:master Aug 6, 2021

ti-srebot mentioned this pull request Aug 6, 2021

Push down ROUND(x) on decimal types (#2492) #2610

Closed

2 tasks

ti-srebot mentioned this pull request Aug 6, 2021

Push down ROUND(x) on decimal types (#2492) #2611

Closed

2 tasks

ToBeVerified unassigned riteme Dec 27, 2021

Conversation

riteme commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

riteme commented Aug 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riteme commented Aug 4, 2021

Uh oh!

riteme commented Aug 4, 2021

Uh oh!

riteme commented Aug 5, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fuzhe1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ti-srebot commented Aug 6, 2021

Uh oh!

ti-srebot commented Aug 6, 2021

Uh oh!

riteme commented Aug 6, 2021

Uh oh!

ti-srebot commented Aug 6, 2021

Uh oh!

ti-srebot commented Aug 6, 2021

Uh oh!

ti-srebot commented Aug 6, 2021

Uh oh!

Reviewers

riteme commented Jul 26, 2021 •

edited

Loading