Skip to content
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

[fix](expr)Remove the _can_fast_execute flag from VExpr. (#45542) #45655

Merged
merged 1 commit into from
Dec 22, 2024

Conversation

Mryange
Copy link
Contributor

@Mryange Mryange commented Dec 19, 2024

#45542
In the past, a _can_fast_execute flag was maintained in VExpr. However, since exec executes concurrently, errors would occur when using the _can_fast_execute judgment.

In fact, we don't need the _can_fast_execute variable because fast_execute will perform the necessary checks itself.

What problem does this PR solve?

Issue Number: close #xxx

Related PR: #xxx

Problem Summary:

Release note

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

In the past, a _can_fast_execute flag was maintained in VExpr.
However, since exec executes concurrently, errors would occur when using
the _can_fast_execute judgment.

In fact, we don't need the _can_fast_execute variable
because fast_execute will perform the necessary checks itself.
@hello-stephen
Copy link
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@Mryange
Copy link
Contributor Author

Mryange commented Dec 19, 2024

run buildall

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

@@ -144,15 +144,13 @@ class VCompoundPred : public VectorizedFnCall {
}

if (all_pass && !res.is_empty()) {
// set fast_execute when expr evaluated by inverted index correctly
_can_fast_execute = true;
context->get_inverted_index_context()->set_inverted_index_result_for_expr(this, res);
}
return Status::OK();
}

Status execute(VExprContext* context, Block* block, int* result_column_id) override {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'execute' has cognitive complexity of 111 (threshold 50) [readability-function-cognitive-complexity]

    Status execute(VExprContext* context, Block* block, int* result_column_id) override {
           ^
Additional context

be/src/vec/exprs/vcompound_pred.h:152: +1, including nesting penalty of 0, nesting level increased to 1

        if (fast_execute(context, block, result_column_id)) {
        ^

be/src/vec/exprs/vcompound_pred.h:155: +1, including nesting penalty of 0, nesting level increased to 1

        if (children().size() == 1 || !_all_child_is_compound_and_not_const()) {
        ^

be/src/vec/exprs/vcompound_pred.h:161: +1, including nesting penalty of 0, nesting level increased to 1

        RETURN_IF_ERROR(_children[0]->execute(context, block, &lhs_id));
        ^

be/src/common/status.h:632: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/vec/exprs/vcompound_pred.h:161: +2, including nesting penalty of 1, nesting level increased to 2

        RETURN_IF_ERROR(_children[0]->execute(context, block, &lhs_id));
        ^

be/src/common/status.h:634: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/vec/exprs/vcompound_pred.h:173: +1, including nesting penalty of 0, nesting level increased to 1

        if (lhs_is_nullable) {
        ^

be/src/vec/exprs/vcompound_pred.h:187: nesting level increased to 1

        auto get_rhs_colum = [&]() {
                             ^

be/src/vec/exprs/vcompound_pred.h:188: +2, including nesting penalty of 1, nesting level increased to 2

            if (rhs_id == -1) {
            ^

be/src/vec/exprs/vcompound_pred.h:189: +3, including nesting penalty of 2, nesting level increased to 3

                RETURN_IF_ERROR(_children[1]->execute(context, block, &rhs_id));
                ^

be/src/common/status.h:632: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/vec/exprs/vcompound_pred.h:189: +4, including nesting penalty of 3, nesting level increased to 4

                RETURN_IF_ERROR(_children[1]->execute(context, block, &rhs_id));
                ^

be/src/common/status.h:634: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/vec/exprs/vcompound_pred.h:199: +3, including nesting penalty of 2, nesting level increased to 3

                if (rhs_is_nullable) {
                ^

be/src/vec/exprs/vcompound_pred.h:207: nesting level increased to 1

        auto return_result_column_id = [&](ColumnPtr res_column, int res_id) -> int {
                                       ^

be/src/vec/exprs/vcompound_pred.h:208: +2, including nesting penalty of 1, nesting level increased to 2

            if (result_is_nullable && !res_column->is_nullable()) {
            ^

be/src/vec/exprs/vcompound_pred.h:208: +1

            if (result_is_nullable && !res_column->is_nullable()) {
                                   ^

be/src/vec/exprs/vcompound_pred.h:217: nesting level increased to 1

        auto create_null_map_column = [&](ColumnPtr& null_map_column,
                                      ^

be/src/vec/exprs/vcompound_pred.h:219: +2, including nesting penalty of 1, nesting level increased to 2

            if (null_map_data == nullptr) {
            ^

be/src/vec/exprs/vcompound_pred.h:228: nesting level increased to 1

        auto vector_vector_null = [&]<bool is_and_op>() {
                                  ^

be/src/vec/exprs/vcompound_pred.h:238: +2, including nesting penalty of 1, nesting level increased to 2

            if constexpr (is_and_op) {
            ^

be/src/vec/exprs/vcompound_pred.h:239: +3, including nesting penalty of 2, nesting level increased to 3

                for (size_t i = 0; i < size; ++i) {
                ^

be/src/vec/exprs/vcompound_pred.h:244: +1, nesting level increased to 2

            } else {
              ^

be/src/vec/exprs/vcompound_pred.h:245: +3, including nesting penalty of 2, nesting level increased to 3

                for (size_t i = 0; i < size; ++i) {
                ^

be/src/vec/exprs/vcompound_pred.h:258: +1, including nesting penalty of 0, nesting level increased to 1

        if (_op == TExprOpcode::COMPOUND_AND) {
        ^

be/src/vec/exprs/vcompound_pred.h:261: +2, including nesting penalty of 1, nesting level increased to 2

            if ((lhs_all_false && !lhs_is_nullable) || (lhs_all_false && lhs_all_is_not_null)) {
            ^

be/src/vec/exprs/vcompound_pred.h:261: +1

            if ((lhs_all_false && !lhs_is_nullable) || (lhs_all_false && lhs_all_is_not_null)) {
                                                    ^

be/src/vec/exprs/vcompound_pred.h:261: +1

            if ((lhs_all_false && !lhs_is_nullable) || (lhs_all_false && lhs_all_is_not_null)) {
                               ^

be/src/vec/exprs/vcompound_pred.h:261: +1

            if ((lhs_all_false && !lhs_is_nullable) || (lhs_all_false && lhs_all_is_not_null)) {
                                                                      ^

be/src/vec/exprs/vcompound_pred.h:264: +1, nesting level increased to 2

            } else {
              ^

be/src/vec/exprs/vcompound_pred.h:265: +3, including nesting penalty of 2, nesting level increased to 3

                RETURN_IF_ERROR(get_rhs_colum());
                ^

be/src/common/status.h:632: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/vec/exprs/vcompound_pred.h:265: +4, including nesting penalty of 3, nesting level increased to 4

                RETURN_IF_ERROR(get_rhs_colum());
                ^

be/src/common/status.h:634: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/vec/exprs/vcompound_pred.h:267: +3, including nesting penalty of 2, nesting level increased to 3

                if ((lhs_all_true && !lhs_is_nullable) ||    //not null column
                ^

be/src/vec/exprs/vcompound_pred.h:267: +1

                if ((lhs_all_true && !lhs_is_nullable) ||    //not null column
                                                       ^

be/src/vec/exprs/vcompound_pred.h:267: +1

                if ((lhs_all_true && !lhs_is_nullable) ||    //not null column
                                  ^

be/src/vec/exprs/vcompound_pred.h:268: +1

                    (lhs_all_true && lhs_all_is_not_null)) { //nullable column
                                  ^

be/src/vec/exprs/vcompound_pred.h:271: +1, nesting level increased to 3

                } else if ((rhs_all_false && !rhs_is_nullable) ||
                       ^

be/src/vec/exprs/vcompound_pred.h:271: +1

                } else if ((rhs_all_false && !rhs_is_nullable) ||
                                                               ^

be/src/vec/exprs/vcompound_pred.h:271: +1

                } else if ((rhs_all_false && !rhs_is_nullable) ||
                                          ^

be/src/vec/exprs/vcompound_pred.h:272: +1

                           (rhs_all_false && rhs_all_is_not_null)) {
                                          ^

be/src/vec/exprs/vcompound_pred.h:275: +1, nesting level increased to 3

                } else if ((rhs_all_true && !rhs_is_nullable) ||
                       ^

be/src/vec/exprs/vcompound_pred.h:275: +1

                } else if ((rhs_all_true && !rhs_is_nullable) ||
                                                              ^

be/src/vec/exprs/vcompound_pred.h:275: +1

                } else if ((rhs_all_true && !rhs_is_nullable) ||
                                         ^

be/src/vec/exprs/vcompound_pred.h:276: +1

                           (rhs_all_true && rhs_all_is_not_null)) {
                                         ^

be/src/vec/exprs/vcompound_pred.h:279: +1, nesting level increased to 3

                } else {
                  ^

be/src/vec/exprs/vcompound_pred.h:280: +4, including nesting penalty of 3, nesting level increased to 4

                    if (!result_is_nullable) {
                    ^

be/src/vec/exprs/vcompound_pred.h:282: +5, including nesting penalty of 4, nesting level increased to 5

                        for (size_t i = 0; i < size; i++) {
                        ^

be/src/vec/exprs/vcompound_pred.h:285: +1, nesting level increased to 4

                    } else {
                      ^

be/src/vec/exprs/vcompound_pred.h:290: +1, nesting level increased to 1

        } else if (_op == TExprOpcode::COMPOUND_OR) {
               ^

be/src/vec/exprs/vcompound_pred.h:293: +2, including nesting penalty of 1, nesting level increased to 2

            if ((lhs_all_true && !lhs_is_nullable) || (lhs_all_true && lhs_all_is_not_null)) {
            ^

be/src/vec/exprs/vcompound_pred.h:293: +1

            if ((lhs_all_true && !lhs_is_nullable) || (lhs_all_true && lhs_all_is_not_null)) {
                                                   ^

be/src/vec/exprs/vcompound_pred.h:293: +1

            if ((lhs_all_true && !lhs_is_nullable) || (lhs_all_true && lhs_all_is_not_null)) {
                              ^

be/src/vec/exprs/vcompound_pred.h:293: +1

            if ((lhs_all_true && !lhs_is_nullable) || (lhs_all_true && lhs_all_is_not_null)) {
                                                                    ^

be/src/vec/exprs/vcompound_pred.h:296: +1, nesting level increased to 2

            } else {
              ^

be/src/vec/exprs/vcompound_pred.h:297: +3, including nesting penalty of 2, nesting level increased to 3

                RETURN_IF_ERROR(get_rhs_colum());
                ^

be/src/common/status.h:632: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/vec/exprs/vcompound_pred.h:297: +4, including nesting penalty of 3, nesting level increased to 4

                RETURN_IF_ERROR(get_rhs_colum());
                ^

be/src/common/status.h:634: expanded from macro 'RETURN_IF_ERROR'

        if (UNLIKELY(!_status_.ok())) { \
        ^

be/src/vec/exprs/vcompound_pred.h:298: +3, including nesting penalty of 2, nesting level increased to 3

                if ((lhs_all_false && !lhs_is_nullable) || (lhs_all_false && lhs_all_is_not_null)) {
                ^

be/src/vec/exprs/vcompound_pred.h:298: +1

                if ((lhs_all_false && !lhs_is_nullable) || (lhs_all_false && lhs_all_is_not_null)) {
                                                        ^

be/src/vec/exprs/vcompound_pred.h:298: +1

                if ((lhs_all_false && !lhs_is_nullable) || (lhs_all_false && lhs_all_is_not_null)) {
                                   ^

be/src/vec/exprs/vcompound_pred.h:298: +1

                if ((lhs_all_false && !lhs_is_nullable) || (lhs_all_false && lhs_all_is_not_null)) {
                                                                          ^

be/src/vec/exprs/vcompound_pred.h:301: +1, nesting level increased to 3

                } else if ((rhs_all_true && !rhs_is_nullable) ||
                       ^

be/src/vec/exprs/vcompound_pred.h:301: +1

                } else if ((rhs_all_true && !rhs_is_nullable) ||
                                                              ^

be/src/vec/exprs/vcompound_pred.h:301: +1

                } else if ((rhs_all_true && !rhs_is_nullable) ||
                                         ^

be/src/vec/exprs/vcompound_pred.h:302: +1

                           (rhs_all_true && rhs_all_is_not_null)) {
                                         ^

be/src/vec/exprs/vcompound_pred.h:305: +1, nesting level increased to 3

                } else if ((rhs_all_false && !rhs_is_nullable) ||
                       ^

be/src/vec/exprs/vcompound_pred.h:305: +1

                } else if ((rhs_all_false && !rhs_is_nullable) ||
                                                               ^

be/src/vec/exprs/vcompound_pred.h:305: +1

                } else if ((rhs_all_false && !rhs_is_nullable) ||
                                          ^

be/src/vec/exprs/vcompound_pred.h:306: +1

                           (rhs_all_false && rhs_all_is_not_null)) {
                                          ^

be/src/vec/exprs/vcompound_pred.h:309: +1, nesting level increased to 3

                } else {
                  ^

be/src/vec/exprs/vcompound_pred.h:310: +4, including nesting penalty of 3, nesting level increased to 4

                    if (!result_is_nullable) {
                    ^

be/src/vec/exprs/vcompound_pred.h:312: +5, including nesting penalty of 4, nesting level increased to 5

                        for (size_t i = 0; i < size; i++) {
                        ^

be/src/vec/exprs/vcompound_pred.h:315: +1, nesting level increased to 4

                    } else {
                      ^

be/src/vec/exprs/vcompound_pred.h:320: +1, nesting level increased to 1

        } else {
          ^

@doris-robot
Copy link

TPC-H: Total hot run time: 40511 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit f7664c9edd77c8cc489d2ac5233ee2f06ef34bda, data reload: false

------ Round 1 ----------------------------------
q1	17613	7419	7290	7290
q2	2061	160	158	158
q3	10716	1081	1240	1081
q4	10551	761	743	743
q5	7750	2847	2715	2715
q6	235	147	142	142
q7	969	639	611	611
q8	9583	1964	1993	1964
q9	7709	6414	6418	6414
q10	6989	2242	2310	2242
q11	457	257	261	257
q12	397	214	207	207
q13	17768	2970	2947	2947
q14	238	206	213	206
q15	560	517	527	517
q16	680	618	603	603
q17	971	580	533	533
q18	7190	6666	6632	6632
q19	1424	1033	1071	1033
q20	471	202	193	193
q21	3942	3232	3051	3051
q22	1103	976	972	972
Total cold run time: 109377 ms
Total hot run time: 40511 ms

----- Round 2, with runtime_filter_mode=off -----
q1	7280	7204	7221	7204
q2	331	230	225	225
q3	2935	2896	2867	2867
q4	1973	1802	1783	1783
q5	5656	5712	5709	5709
q6	233	141	144	141
q7	2209	1775	1776	1775
q8	3289	3516	3449	3449
q9	8764	8798	8787	8787
q10	3553	3525	3486	3486
q11	609	501	496	496
q12	775	596	591	591
q13	16485	3121	3125	3121
q14	298	268	278	268
q15	558	520	535	520
q16	702	674	681	674
q17	1856	1609	1617	1609
q18	8169	7805	7517	7517
q19	6253	1538	1619	1538
q20	2066	1865	1841	1841
q21	5314	5218	5175	5175
q22	1081	992	1014	992
Total cold run time: 80389 ms
Total hot run time: 59768 ms

@doris-robot
Copy link

TPC-DS: Total hot run time: 195018 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit f7664c9edd77c8cc489d2ac5233ee2f06ef34bda, data reload: false

query1	1307	934	916	916
query2	6245	2077	2004	2004
query3	10813	4050	4038	4038
query4	66520	29477	23377	23377
query5	5308	426	443	426
query6	442	167	163	163
query7	5645	307	308	307
query8	308	230	223	223
query9	9534	2681	2655	2655
query10	505	279	263	263
query11	17801	15159	15885	15159
query12	161	104	109	104
query13	1528	425	427	425
query14	10980	7328	7259	7259
query15	212	178	179	178
query16	7311	506	496	496
query17	1280	578	572	572
query18	1896	326	317	317
query19	245	152	151	151
query20	117	108	115	108
query21	71	47	45	45
query22	4653	4415	4337	4337
query23	34512	33972	33888	33888
query24	6257	2924	2972	2924
query25	514	420	421	420
query26	687	166	166	166
query27	1895	290	307	290
query28	4378	2564	2530	2530
query29	710	466	462	462
query30	250	167	168	167
query31	1007	832	852	832
query32	65	52	53	52
query33	466	274	273	273
query34	899	498	495	495
query35	848	740	741	740
query36	1097	968	950	950
query37	118	68	75	68
query38	4104	4058	4143	4058
query39	1529	1491	1479	1479
query40	137	80	81	80
query41	47	45	44	44
query42	108	96	104	96
query43	521	484	482	482
query44	1169	790	797	790
query45	182	178	166	166
query46	1171	728	752	728
query47	2002	1892	1887	1887
query48	489	365	368	365
query49	720	384	369	369
query50	840	437	427	427
query51	7382	7025	7158	7025
query52	99	82	89	82
query53	252	178	178	178
query54	552	450	431	431
query55	76	73	75	73
query56	249	231	222	222
query57	1175	1076	1076	1076
query58	200	203	194	194
query59	3152	2927	2780	2780
query60	266	250	242	242
query61	103	103	106	103
query62	755	644	636	636
query63	205	189	191	189
query64	1702	636	654	636
query65	3262	3209	3156	3156
query66	724	293	294	293
query67	15647	15262	15192	15192
query68	4900	546	543	543
query69	442	250	242	242
query70	1162	1070	1112	1070
query71	401	239	242	239
query72	6564	3904	3910	3904
query73	764	340	334	334
query74	10364	8825	9082	8825
query75	3342	2624	2629	2624
query76	2059	998	1105	998
query77	492	259	264	259
query78	10704	9686	9410	9410
query79	7273	571	576	571
query80	1795	421	429	421
query81	553	238	232	232
query82	1093	112	123	112
query83	256	145	140	140
query84	292	80	82	80
query85	1721	299	282	282
query86	475	294	293	293
query87	4414	4186	4231	4186
query88	5440	2422	2396	2396
query89	416	284	293	284
query90	2095	186	191	186
query91	181	144	144	144
query92	62	47	49	47
query93	5908	535	536	535
query94	899	274	271	271
query95	345	250	259	250
query96	625	280	281	280
query97	3345	3171	3122	3122
query98	218	198	194	194
query99	1657	1285	1315	1285
Total cold run time: 335662 ms
Total hot run time: 195018 ms

@yiguolei yiguolei merged commit 66d5708 into apache:branch-3.0 Dec 22, 2024
25 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants