least/greatest function for string#6662

Merged

ti-chi-bot merged 23 commits intopingcap:masterfrom

ywqzzy:least_greatest_string

Feb 8, 2023

Contributor

ywqzzy commented Jan 19, 2023 •

edited

Loading

What problem does this PR solve?

Issue Number: close #6660

Problem Summary:

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None


          init

7ae8271

Member

ti-chi-bot commented Jan 19, 2023 •

edited

Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

SeaRise
gengliqi

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 added do-not-merge/needs-linked-issue release-note-none size/XXL and removed do-not-merge/needs-linked-issue labels


          remove useless code.

b7989eb

Lloyd-Pottiger reviewed

View reviewed changes

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

dbms/src/Functions/LeastGreatest.h Outdated Show resolved Hide resolved

dbms/src/Functions/LeastGreatest.h Outdated Show resolved Hide resolved

yibin87 reviewed

View reviewed changes

Contributor

yibin87 left a comment

Add fullstack sql test please.

dbms/src/Functions/LeastGreatest.h Outdated Show resolved Hide resolved

ywqzzy changed the title ~~least/greast function for string~~ least/greatest function for string

ywqzzy added 5 commits

February 6, 2023 12:38


          tmp save


          pass all tests

7ee124c


          add ft

fd3cd49


          refine

94b59d3


          refine

0e65d0d

ywqzzy requested review from Lloyd-Pottiger, gengliqi and yibin87

February 7, 2023 01:03

ywqzzy added 2 commits

February 7, 2023 09:06


          refine comments

03d3807


          refine include

3ff83b4

ywqzzy force-pushed the least_greatest_string branch from 337b187 to 3ff83b4 Compare

February 7, 2023 01:08


          Merge branch 'master' into least_greatest_string

ceb66d4

Contributor Author

ywqzzy commented Feb 7, 2023

/run-all-tests

Contributor Author

ywqzzy commented Feb 7, 2023

/run-unit-tests

ywqzzy requested a review from xzhangxian1008

February 7, 2023 05:13

xzhangxian1008 reviewed

View reviewed changes

dbms/src/Functions/tests/gtest_least_greatest.cpp Show resolved Hide resolved

dbms/src/Functions/tests/gtest_least_greatest.cpp Outdated Show resolved Hide resolved

dbms/src/Functions/tests/gtest_least_greatest.cpp Show resolved Hide resolved

gengliqi reviewed

View reviewed changes

dbms/src/Functions/LeastGreatest.h Outdated Show resolved Hide resolved

dbms/src/Functions/LeastGreatest.h Outdated Show resolved Hide resolved

dbms/src/Functions/LeastGreatest.h Outdated Show resolved Hide resolved

dbms/src/Functions/LeastGreatest.h Outdated Show resolved Hide resolved

gengliqi reviewed

View reviewed changes

dbms/src/Functions/LeastGreatest.h Outdated Show resolved Hide resolved

xzhangxian1008 reviewed

View reviewed changes

dbms/src/Functions/LeastGreatest.h Outdated Show resolved Hide resolved

dbms/src/Functions/LeastGreatest.h Outdated Show resolved Hide resolved

dbms/src/Functions/LeastGreatest.h Outdated Show resolved Hide resolved

dbms/src/Functions/LeastGreatest.h Outdated Show resolved Hide resolved

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


          address comments, optimize memcpy

6e3ea6d


          address comments.

709cac2

ywqzzy requested review from gengliqi and xzhangxian1008

February 8, 2023 06:53

Contributor

xzhangxian1008 commented Feb 8, 2023

lgtm


          update license

15c3dfe

SeaRise approved these changes

View reviewed changes

Contributor

SeaRise left a comment

LGTM by xzhangxian

ti-chi-bot added the status/LGT1 label

gengliqi approved these changes

View reviewed changes

dbms/src/Functions/LeastGreatest.h Outdated Show resolved Hide resolved

dbms/src/Functions/LeastGreatest.h Outdated

+                      }
+                      // 3B. merge result columns of const columns and result_col
+                      if (string_columns_size == 1 && !const_columns.empty())

Contributor

gengliqi Feb 8, 2023

Move to upper if (string_columns_size == 1) ? Actually, if string_columns_size is 1, const_columns must be non-empty because the arguments size is greater than 2.

ti-chi-bot added status/LGT2 and removed status/LGT1 labels

Contributor Author

ywqzzy commented Feb 8, 2023

/merge

Member

ti-chi-bot commented Feb 8, 2023

@ywqzzy: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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 ti-community-infra/tichi repository.

Member

ti-chi-bot commented Feb 8, 2023

This pull request has been accepted and is ready to merge.

Details

Commit hash: 2ccea27

ti-chi-bot added the status/can-merge label

Contributor Author

ywqzzy commented Feb 8, 2023

/hold

ti-chi-bot added the do-not-merge/hold label

ywqzzy added 2 commits

February 8, 2023 17:05


          Merge branch 'master' of https://github.com/pingcap/tiflash into leas…

d780b5c

…t_greatest_string


          address comments

b51b895

ywqzzy force-pushed the least_greatest_string branch from 3931557 to b51b895 Compare

February 8, 2023 09:11

ti-chi-bot removed the status/can-merge label

Contributor Author

ywqzzy commented Feb 8, 2023

/unhold

ti-chi-bot removed the do-not-merge/hold label

Contributor Author

ywqzzy commented Feb 8, 2023

/merge

Member

ti-chi-bot commented Feb 8, 2023

@ywqzzy: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

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 ti-community-infra/tichi repository.

Member

ti-chi-bot commented Feb 8, 2023

This pull request has been accepted and is ready to merge.

Details

Commit hash: b51b895

ti-chi-bot added the status/can-merge label

Contributor Author

ywqzzy commented Feb 8, 2023

/run-unit-tests

ti-chi-bot merged commit f147859 into pingcap:master

guo-shaoge pushed a commit to guo-shaoge/tiflash that referenced this pull request


          least/greatest function for string (pingcap#6662)

a6daf46

close pingcap#6660

ywqzzy added a commit to ywqzzy/tiflash_1 that referenced this pull request


          least/greatest function for string (pingcap#6662)

4e8d098

close pingcap#6660

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Lloyd-Pottiger Lloyd-Pottiger left review comments

gengliqi gengliqi approved these changes

yibin87 Awaiting requested review from yibin87

xzhangxian1008 Awaiting requested review from xzhangxian1008

+1 more reviewer

SeaRise SeaRise approved these changes

Labels

release-note-none size/XXL status/can-merge status/LGT2