expression: support least/greatest for int/real.#3801
expression: support least/greatest for int/real.#3801ti-chi-bot merged 31 commits intopingcap:masterfrom
Conversation
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. DetailsReviewer can indicate their review by submitting an approval review. |
|
How to make a benchmark? The benchmark result: After I addressed the comments, The benchmark result is as follow:After I addressed the comments the second time, The benchmark result is as follow:The row num is 10000. |
dbms/src/Functions/LeastGreatest.h
Outdated
| enum class LeastGreatest | ||
| { | ||
| Least, | ||
| Greatest | ||
| }; |
There was a problem hiding this comment.
enum class LeastGreatest
{
Least,
Greatest
};
how about convert it to a trait class like:
struct LeastImpl
{
static constexpr auto name = "tidbLeast";
using ReturnType = xxx;
};
struct GreatestImpl
{
static constexpr auto name = "tidbGreatest";
using ReturnType = xxx;
};There was a problem hiding this comment.
I am not sure how to use the ReturnType. But I will change it according to the comments in this closed pr #3537
dbms/src/DataTypes/NumberTraits.h
Outdated
| using Type = std::conditional_t< | ||
| std::is_floating_point_v<A> || std::is_floating_point_v<B>, | ||
| Float64, | ||
| std::conditional_t<sizeof(A) == 8 && sizeof(A) == sizeof(B), typename BinaryLeastSpecialCase<A, B>::Type, Int64>>; |
There was a problem hiding this comment.
guess we don't need to check the size, but limit the types to integer/float is necessary.
There was a problem hiding this comment.
Need it. If remove the size check,the test below won't pass, the output column will be UINT64, because tidb
don't cast every int to int64.
ASSERT_COLUMN_EQ(
createColumn<Int64>({7, 2, 3, 3, 2}),
executeFunction(
func_name,
createColumn<UInt16>({10, 2, 3, 4, 5}),
createColumn<UInt16>({7, 6, 5, 3, 4}),
createColumn<UInt16>({8, 9, 6, 3, 2})));
There was a problem hiding this comment.
in TiDB it does return UInt64.
dbms/src/Functions/LeastGreatest.h
Outdated
| @@ -188,15 +187,21 @@ class FunctionRowbasedLeastGreatest : public IFunction | |||
|
|
|||
| if (checkDataType<DataTypeInt64>(result_type.get())) | |||
There was a problem hiding this comment.
(may not be critical) another way to get the type is:
TypeIndex type_index = removeNullable(block.getByPosition(arguments[1]).type)->getTypeId();
switch (type_index)
...However I'm not sure which way is better.
There was a problem hiding this comment.
There was a problem hiding this comment.
Maybe I can extract the type_index way to a function.
In function callOnBasicTypes(), I have found the similar logic
dbms/src/DataTypes/NumberTraits.h
Outdated
| using Type = std::conditional_t< | ||
| std::is_floating_point_v<A> || std::is_floating_point_v<B>, | ||
| Float64, | ||
| std::conditional_t<sizeof(A) == 8 && sizeof(A) == sizeof(B), typename BinaryLeastSpecialCase<A, B>::Type, Int64>>; |
There was a problem hiding this comment.
in TiDB it does return UInt64.
dbms/src/Functions/greatest.cpp
Outdated
| static Result apply(A a, B b) | ||
| { | ||
| return static_cast<Result>(a) > static_cast<Result>(b) ? static_cast<Result>(a) : static_cast<Result>(b); | ||
| return accurate::greaterOp(a, b) ? static_cast<Result>(a) : static_cast<Result>(b); |
There was a problem hiding this comment.
Why not cast a/b to the ResultType before comparision?
Signed-off-by: guo-shaoge <shaoge1994@163.com> Conflicts: dbms/CMakeLists.txt
| {tipb::ScalarFuncSig::GreatestReal, "greatest"}, | ||
| {tipb::ScalarFuncSig::GreatestInt, "tidbGreatest"}, | ||
| {tipb::ScalarFuncSig::GreatestReal, "tidbGreatest"}, | ||
| {tipb::ScalarFuncSig::GreatestString, "greatest"}, |
There was a problem hiding this comment.
Because greatest/least(bigint unsigned , bigint) will use Decimal. So we need to enable GreatestDecimal too.
There was a problem hiding this comment.
Do you mean greatest/least(bigint unsigned , bigint) will be rewritten to greatest/least(cast(bigint unsigned as decimal(20,0)) , cast(bigint as decimal(20,0)) in TiDB?
dbms/src/Functions/greatest.cpp
Outdated
| { | ||
| factory.registerFunction<FunctionGreatest>(); | ||
| factory.registerFunction<FunctionTiDBGreatest>(); | ||
| factory.registerFunction<FunctionBinaryGreatest>(); |
There was a problem hiding this comment.
Maybe no need to register FunctionBinaryGreatest ?
| struct LeastBaseImpl<A, B, true> | ||
| struct BinaryLeastBaseImpl<A, B, true> | ||
| { | ||
| using ResultType = If<std::is_floating_point_v<A> || std::is_floating_point_v<B>, double, Decimal32>; |
There was a problem hiding this comment.
Maybe Decimal32 is not enough in some situation?
There was a problem hiding this comment.
Since greatest/least(decimal) is not supported yet, we can ignore this.
Signed-off-by: guo-shaoge <shaoge1994@163.com>
|
/run-check-issue-triage-complete |
|
/run-integration-test |
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
|
/merge |
|
@windtalker: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
This pull request has been accepted and is ready to merge. DetailsCommit hash: 06ea3c2 |
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
What problem does this PR solve?
Issue Number: close #3358
Problem Summary:
What is changed and how it works?
Check List
Tests
Release note