-
Notifications
You must be signed in to change notification settings - Fork 409
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
Push down ROUND(x)
on decimal types
#2492
Conversation
ROUND
on decimal typesROUND(x)
on decimal types
ROUND(x)
on decimal typesROUND(x)
on decimal types
/run-all-tests |
if (expr.children_size() != 1) | ||
throw TiFlashException("Invalid arguments of ROUND function", Errors::Coprocessor::BadRequest); | ||
|
||
Names argument_names; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can construct argument_names
after const_zero_arg_name
is ready.
dbms/src/Functions/FunctionsRound.h
Outdated
block.getByPosition(result).column = std::move(result_column); | ||
} | ||
|
||
bool checkInputType(const DataTypePtr & input_type, const DataTypePtr & return_type, const DataTypePtr & frac_type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
using ResultColumn = std::conditional_t<IsDecimal<ReturnType>, ColumnDecimal<ReturnType>, ColumnVector<ReturnType>>; | ||
|
||
// TODO: RoundWithFrac | ||
EXPECT(!input_column->isColumnConst()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guess I'm reviewing an unfinished pr...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah...It's unfinished because the code framework is prepared for round(x, d)
.
dbms/src/Functions/FunctionsRound.h
Outdated
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Any suggestion on naming? What about
checkInputTypeAndApply
? - I will rewrite the code that throws an exception in each
checkXXX
function.
{tipb::ScalarFuncSig::RoundReal, "tidbRound"}, | ||
{tipb::ScalarFuncSig::RoundInt, "tidbRound"}, | ||
{tipb::ScalarFuncSig::RoundDec, "tidbRound"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
ROUND(x)
on decimal typesROUND(x)
on decimal types
/run-all-tests |
1 similar comment
/run-all-tests |
/run-all-tests |
dbms/src/Functions/FunctionsRound.h
Outdated
// for DEBUG only, will be removed after RoundWithFrac is implemented. | ||
#define EXPECT(expr) \ | ||
if (!(expr)) \ | ||
throw TiFlashException(fmt::format("\"{}\" failed", #expr), Errors::Coprocessor::Internal); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to replace them with assert
so that they don't abort in Release mode.
dbms/src/Functions/FunctionsRound.h
Outdated
} | ||
|
||
// in MySQL, frac is clamped to 30, which is identical to decimal_max_scale. | ||
if (unsigned_frac > decimal_max_scale) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the result won't be clamped if the field is signed, is it expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's indeed a bug.
@@ -874,7 +869,7 @@ struct TiDBIntegerRound | |||
static constexpr InputType eval(const InputType & input, FracType frac) | |||
{ | |||
// TODO: RoundWithFrac. | |||
EXPECT(frac == 0); | |||
assert(frac == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then how will you prevent round(x, frac)
from being pushed down?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only tidbRound
is mapped in DAGUtils.cpp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest LGTM
dbms/src/Functions/FunctionsRound.h
Outdated
{ | ||
static_assert(is_integer_v<InputType>); | ||
|
||
static constexpr InputType eval(const InputType & input, FracType frac) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here [[maybe_unused]]
dbms/src/Functions/FunctionsRound.h
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here [[maybe_unused]]
@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). |
1 similar comment
@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). |
/merge |
/run-all-tests |
cherry pick to release-5.0 in PR #2610 |
cherry pick to release-5.1 in PR #2611 |
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
: addisFloatingPoint
.DAGExpressionAnalyzer.cpp
: constructtidbRound
fromtidbRoundWithFrac
.DAGUtils.cpp
: map TiPB codes to function names.FunctionsArithmetic.h
,toUnsigned.h
: move outtoUnsigned
for reusing inFunctionsRound.h
.FunctionsRound.cpp
: register new function.FunctionsRound.h
: the implementation.Related changes
pingcap/docs
/pingcap/docs-cn
:Check List
Tests
Release note
ROUND(X)
to TiFlash.