[WIP] support least() function. by ywqzzy · Pull Request #3537 · pingcap/tiflash

ywqzzy · 2021-11-23T07:54:13Z

What problem does this PR solve?

Issue Number: close #xxx

Problem Summary:

What is changed and how it works?

Proposal: xxx

What's Changed:

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
Manual test (add detailed scripts or steps below)
No code

Side effects

Release note

Please add a release note, or a 'None' if it is not needed.

ti-chi-bot · 2021-11-23T07:54:14Z

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

ti-chi-bot · 2021-12-18T08:29:29Z

@ywqzzy: PR needs rebase.

Details

Instructions 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 kubernetes/test-infra repository.

SeaRise · 2021-12-29T10:40:53Z

dbms/src/Functions/FunctionBinaryArithmetic.h

+                    c_res->getChars(),
+                    c_res->getOffsets());
+            else
+                throw Exception("Illegal columns "


fmt::format

SeaRise · 2021-12-29T10:41:06Z

dbms/src/Functions/FunctionBinaryArithmetic.h

+                    c_res->getChars(),
+                    c_res->getOffsets());
+            else
+                throw Exception("Illegal columns "


SeaRise · 2021-12-29T10:44:13Z

dbms/src/Functions/LeastGreatestGeneric.h

+class FunctionTiDBLeastGreatest : public IFunction
+{
+public:
+    static constexpr auto name = kind == LeastGreatest::Least ? "tidbLeast" : "tidbGreatest";


how about `` static constexpr auto name = kind::name`

SeaRise · 2021-12-29T10:44:26Z

dbms/src/Functions/LeastGreatestGeneric.h

+    DataTypePtr getReturnTypeImpl(const DataTypes & arguments) const override
+    {
+        if (arguments.size() < 2)
+            throw Exception("Number of arguments for function " + getName() + " doesn't match: passed " + toString(arguments.size())


fmt::format

SeaRise · 2021-12-29T10:45:11Z

dbms/src/Functions/LeastGreatestGeneric.h

+        size_t num_arguments = arguments.size();
+        if (num_arguments < 2)
+        {
+            throw Exception("Number of arguments for function " + getName() + " doesn't match: passed "


fmt::format

SeaRise · 2021-12-29T10:47:28Z

dbms/src/Functions/LeastGreatestGeneric.h

+    DataTypePtr getReturnTypeImpl(const DataTypes & arguments) const override
+    {
+        if (arguments.size() < 2)
+            throw Exception("Number of arguments for function " + getName() + " doesn't match: passed " + toString(arguments.size())


SeaRise · 2021-12-29T10:47:44Z

dbms/src/Functions/LeastGreatestGeneric.h

+        size_t num_arguments = arguments.size();
+        if (num_arguments < 2)
+        {
+            throw Exception("Number of arguments for function " + getName() + " doesn't match: passed "


SeaRise · 2021-12-29T10:49:46Z

dbms/src/Columns/ColumnConst.h

        return data->compareAt(0, 0, *static_cast<const ColumnConst &>(rhs).data, nan_direction_hint);
    }

+    int compareAtWithCollation(size_t, size_t m, const IColumn & rhs_, int null_direction_hint, const ICollator & collator) const override


how about int int compareAt(size_t, size_t, const IColumn & rhs, int nan_direction_hint, , const ICollator * collator = nullptr) const override

maybe i can create another pr to make the interface clean in all related files.

pr is #3790, PTAL

SeaRise · 2021-12-29T10:51:05Z

dbms/src/Functions/LeastGreatestGeneric.h

+                else
+                    cmp_result = converted_columns[arg]->compareAt(row_num, row_num, *converted_columns[best_arg], 1);
+
+                if constexpr (kind == LeastGreatest::Least)


how about if (kind::xxx(cmp_result )) { best_arg = arg; }

SeaRise · 2021-12-29T10:51:18Z

dbms/src/Functions/LeastGreatestGeneric.h

+    DataTypePtr getReturnTypeImpl(const DataTypes & arguments) const override
+    {
+        if (arguments.size() < 2)
+            throw Exception("Number of arguments for function " + getName() + " doesn't match: passed " + toString(arguments.size())


JaySon-Huang · 2022-01-04T04:08:08Z

dbms/src/Flash/Coprocessor/DAGUtils.cpp


 TiDB::TiDBCollatorPtr getCollatorFromFieldType(const tipb::FieldType & field_type)
 {
+    std::cout << "field type collate: " << field_type.collate() << std::endl;


Suggested change

std::cout << "field type collate: " << field_type.collate() << std::endl;

JaySon-Huang · 2022-01-04T04:08:33Z

dbms/src/Flash/Coprocessor/DAGExpressionAnalyzer.cpp

    for (const auto & child : expr.children())
    {
        String name = getActions(child, actions);
+        std::cout << "name: " << name << std::endl;


clean up these debug lines

JaySon-Huang · 2022-01-04T04:08:38Z

dbms/src/Flash/Coprocessor/DAGExpressionAnalyzer.cpp

    ExpressionActionsPtr & actions)
 {
    const String & func_name = getFunctionName(expr);
+    std::cout << "func_name: " << func_name << std::endl;


JaySon-Huang · 2022-01-04T04:08:44Z

dbms/src/Flash/Coprocessor/DAGExpressionAnalyzer.cpp

    const TiDB::TiDBCollatorPtr & collator)
 {
    String result_name = genFuncString(func_name, arg_names, {collator});
+    std::cout << "result name: " << result_name << std::endl;


fuzhe1989 · 2022-01-04T06:24:21Z

dbms/src/Common/Decimal.h


+struct CompareDecimalInferer
+{
+    static inline void infer(PrecType left_prec, ScaleType left_scale, PrecType right_prec, ScaleType right_scale, PrecType & result_prec, ScaleType & result_scale)


Suggested change

static inline void infer(PrecType left_prec, ScaleType left_scale, PrecType right_prec, ScaleType right_scale, PrecType & result_prec, ScaleType & result_scale)

static std::tuple<PrecType, ScaleType> infer(PrecType left_prec, ScaleType left_scale, PrecType right_prec, ScaleType right_scale)

what is it used for?

fuzhe1989 · 2022-01-04T06:26:18Z

dbms/src/Common/MyTime.h

    {}

    String toString(int fsp) const;
+    int fsp;


could you explain why MyDateTime need fsp and why not put it into the base class?

ASSERT_COLUMN_EQ( createColumn<Nullable<String>>({"2020-10-20 00:00:00"}), executeFunction( func_name, createColumn<Nullable<String>>({"2021-12-28 11:19:09"}), createColumn<Nullable<String>>({"2020-10-20"})));

Without fsp, the result will be "2020-10-20", which is inconsistent with MYSQL and TIDB, the follwing getFsp() function was added to align with TIDB

fuzhe1989 · 2022-01-04T06:28:00Z

dbms/src/Common/MyTime.cpp

    return idx;
 }

+int getFsp(const String & s)


which format is s expected to be?

fuzhe1989 · 2022-01-04T06:33:23Z

dbms/src/DataTypes/NumberTraits.h

+template <typename A, typename B>
+struct ResultOfTiDBLeast
+{
+    static constexpr auto result_size = std::max(actual_size_v<A>, actual_size_v<B>);


to align with TiDB, least/greatest for integers should always return Int64 or UInt64.

fuzhe1989 · 2022-01-04T06:35:40Z

dbms/src/DataTypes/NumberTraits.h

    typename ResultOfIf<A, B>::Type>;

+template <typename A, typename B>
+constexpr bool TiDBLeastGreatestSpecialCase


It should be a type inferer and let the users get the return type of least/greatest like:

using ReturnType = TiDBLeastGreatestReturnTypeInferer<true /*least*/, A, B>;

type check.

d501cef

ywqzzy added 5 commits November 23, 2021 16:00

format..

6c209fe

update.

a66f326

format.

e462b7c

update..

b00fd4e

poc done..

a36f99b

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 24, 2021

ywqzzy added 6 commits November 25, 2021 15:02

update.

c944343

fix type error.

97be5be

tests.

3ad147a

fix decimal's bug.

26692b7

add test

2e4459a

format.

b300b23

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 1, 2021

ywqzzy added 10 commits December 1, 2021 19:44

fix uint64 special case.

fd54c3e

format.

3d91c72

update. support greatest.

4f6e3cc

add tests.

1241ada

update.

1e112bb

format.

68f450a

half way support string, can't compile yet..

6b9856d

fix compile error, need tests to make sure the logic is right.

2bfa6f4

update.

24b042f

pass collator.

9cfe919

ywqzzy added 9 commits December 14, 2021 20:14

update.

57362d7

update.

1c48434

Merge branch 'master' of https://github.com/pingcap/tics into ywq_least

9e4873f

update

977bc2e

update.

b700776

Merge branch 'master' of https://github.com/pingcap/tics into ywq_least

467ee34

update.

e8772c5

format.

f1d4242

introduce bench success....

9892d3f

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2021

ywqzzy added 8 commits December 20, 2021 10:51

StringOp without collator.

6165c3b

update, add nullable bench.

a0329b9

update.

3a33466

clean bench.

8ef091e

clean bench.

9774b0d

update.

db22efc

update.

8692cbe

half way support date...

08631e5

SeaRise self-requested a review December 29, 2021 03:29

ywqzzy added 2 commits December 29, 2021 15:48

support date POC and some refactor.

21db418

Merge branch 'master' of https://github.com/pingcap/tics into ywq_least

ece6f8f

SeaRise reviewed Dec 29, 2021

View reviewed changes

SeaRise self-requested a review December 29, 2021 10:54

resolve comments.

e3f1635

JaySon-Huang reviewed Jan 4, 2022

View reviewed changes

fuzhe1989 reviewed Jan 4, 2022

View reviewed changes

ywqzzy mentioned this pull request Jan 5, 2022

expression: support least/greatest for int/real. #3801

Merged

4 tasks

ywqzzy closed this May 26, 2022

	static inline void infer(PrecType left_prec, ScaleType left_scale, PrecType right_prec, ScaleType right_scale, PrecType & result_prec, ScaleType & result_scale)
	static std::tuple<PrecType, ScaleType> infer(PrecType left_prec, ScaleType left_scale, PrecType right_prec, ScaleType right_scale)

Conversation

ywqzzy commented Nov 23, 2021

What problem does this PR solve?

What is changed and how it works?

Related changes

Check List

Release note

Uh oh!

ti-chi-bot commented Nov 23, 2021

Uh oh!

ti-chi-bot commented Dec 18, 2021

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!

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!

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!

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!

ywqzzy Jan 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ywqzzy Jan 4, 2022 •

edited

Loading