Skip to content

Conversation

@BiteTheDDDDt
Copy link
Contributor

@BiteTheDDDDt BiteTheDDDDt commented Oct 10, 2024

Proposed changes

send ignored rf when hash join build closed early to avoid runtime filter sync/merge error
Follow-up : #41292

@BiteTheDDDDt
Copy link
Contributor Author

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

std::vector<vectorized::VRuntimeFilterPtr>& push_exprs,
const TExpr& probe_expr);

Status merge(const RuntimePredicateWrapper* wrapper) {
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 'merge' has cognitive complexity of 74 (threshold 50) [readability-function-cognitive-complexity]

    Status merge(const RuntimePredicateWrapper* wrapper) {
           ^
Additional context

be/src/exprs/runtime_filter.cpp:474: +1, including nesting penalty of 0, nesting level increased to 1

        if (wrapper->is_ignored()) {
        ^

be/src/exprs/runtime_filter.cpp:480: +1

                _filter_type == RuntimeFilterType::IN_OR_BLOOM_FILTER &&
                                                                      ^

be/src/exprs/runtime_filter.cpp:485: +1

        bool can_not_merge_other = _filter_type != RuntimeFilterType::IN_OR_BLOOM_FILTER &&
                                                                                         ^

be/src/exprs/runtime_filter.cpp:488: +1

        CHECK(!can_not_merge_in_or_bloom && !can_not_merge_other)
                                         ^

be/src/exprs/runtime_filter.cpp:493: +1, including nesting penalty of 0, nesting level increased to 1

        switch (_filter_type) {
        ^

be/src/exprs/runtime_filter.cpp:497: +2, including nesting penalty of 1, nesting level increased to 2

            if (_max_in_num >= 0 && _context->hybrid_set->size() >= _max_in_num) {
            ^

be/src/exprs/runtime_filter.cpp:507: +2, including nesting penalty of 1, nesting level increased to 2

            RETURN_IF_ERROR(_context->minmax_func->merge(wrapper->_context->minmax_func.get()));
            ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:507: +3, including nesting penalty of 2, nesting level increased to 3

            RETURN_IF_ERROR(_context->minmax_func->merge(wrapper->_context->minmax_func.get()));
            ^

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

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

be/src/exprs/runtime_filter.cpp:511: +2, including nesting penalty of 1, nesting level increased to 2

            RETURN_IF_ERROR(
            ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:511: +3, including nesting penalty of 2, nesting level increased to 3

            RETURN_IF_ERROR(
            ^

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

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

be/src/exprs/runtime_filter.cpp:519: +2, including nesting penalty of 1, nesting level increased to 2

            if (other_filter_type == RuntimeFilterType::IN_OR_BLOOM_FILTER) {
            ^

be/src/exprs/runtime_filter.cpp:523: +2, including nesting penalty of 1, nesting level increased to 2

            if (real_filter_type == RuntimeFilterType::IN_FILTER) {
            ^

be/src/exprs/runtime_filter.cpp:527: +3, including nesting penalty of 2, nesting level increased to 3

                if (other_filter_type == RuntimeFilterType::IN_FILTER) {
                ^

be/src/exprs/runtime_filter.cpp:529: +4, including nesting penalty of 3, nesting level increased to 4

                    if (_max_in_num >= 0 && _context->hybrid_set->size() >= _max_in_num) {
                    ^

be/src/exprs/runtime_filter.cpp:531: +5, including nesting penalty of 4, nesting level increased to 5

                        RETURN_IF_ERROR(_context->bloom_filter_func->init_with_fixed_length());
                        ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:531: +6, including nesting penalty of 5, nesting level increased to 6

                        RETURN_IF_ERROR(_context->bloom_filter_func->init_with_fixed_length());
                        ^

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

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

be/src/exprs/runtime_filter.cpp:532: +5, including nesting penalty of 4, nesting level increased to 5

                        RETURN_IF_ERROR(change_to_bloom_filter());
                        ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:532: +6, including nesting penalty of 5, nesting level increased to 6

                        RETURN_IF_ERROR(change_to_bloom_filter());
                        ^

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

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

be/src/exprs/runtime_filter.cpp:534: +1, nesting level increased to 3

                } else {
                  ^

be/src/exprs/runtime_filter.cpp:537: +4, including nesting penalty of 3, nesting level increased to 4

                    RETURN_IF_ERROR(change_to_bloom_filter());
                    ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:537: +5, including nesting penalty of 4, nesting level increased to 5

                    RETURN_IF_ERROR(change_to_bloom_filter());
                    ^

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

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

be/src/exprs/runtime_filter.cpp:539: +1, nesting level increased to 2

            } else {
              ^

be/src/exprs/runtime_filter.cpp:540: +3, including nesting penalty of 2, nesting level increased to 3

                if (other_filter_type == RuntimeFilterType::IN_FILTER) {
                ^

be/src/exprs/runtime_filter.cpp:543: +1, nesting level increased to 3

                } else {
                  ^

be/src/exprs/runtime_filter.cpp:545: +4, including nesting penalty of 3, nesting level increased to 4

                    RETURN_IF_ERROR(_context->bloom_filter_func->merge(
                    ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:545: +5, including nesting penalty of 4, nesting level increased to 5

                    RETURN_IF_ERROR(_context->bloom_filter_func->merge(
                    ^

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

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

@BiteTheDDDDt
Copy link
Contributor Author

run buildall

@BiteTheDDDDt
Copy link
Contributor Author

run buildall

@BiteTheDDDDt
Copy link
Contributor Author

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

std::vector<vectorized::VRuntimeFilterPtr>& push_exprs,
const TExpr& probe_expr);

Status merge(const RuntimePredicateWrapper* wrapper) {
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 'merge' has cognitive complexity of 76 (threshold 50) [readability-function-cognitive-complexity]

    Status merge(const RuntimePredicateWrapper* wrapper) {
           ^
Additional context

be/src/exprs/runtime_filter.cpp:474: +1, including nesting penalty of 0, nesting level increased to 1

        if (wrapper->is_ignored()) {
        ^

be/src/exprs/runtime_filter.cpp:480: +1

                _filter_type == RuntimeFilterType::IN_OR_BLOOM_FILTER &&
                                                                      ^

be/src/exprs/runtime_filter.cpp:485: +1

        bool can_not_merge_other = _filter_type != RuntimeFilterType::IN_OR_BLOOM_FILTER &&
                                                                                         ^

be/src/exprs/runtime_filter.cpp:488: +1

        CHECK(!can_not_merge_in_or_bloom && !can_not_merge_other)
                                         ^

be/src/exprs/runtime_filter.cpp:493: +1, including nesting penalty of 0, nesting level increased to 1

        switch (_filter_type) {
        ^

be/src/exprs/runtime_filter.cpp:496: +2, including nesting penalty of 1, nesting level increased to 2

            if (!_context->hybrid_set) {
            ^

be/src/exprs/runtime_filter.cpp:500: +2, including nesting penalty of 1, nesting level increased to 2

            if (_max_in_num >= 0 && _context->hybrid_set->size() >= _max_in_num) {
            ^

be/src/exprs/runtime_filter.cpp:510: +2, including nesting penalty of 1, nesting level increased to 2

            RETURN_IF_ERROR(_context->minmax_func->merge(wrapper->_context->minmax_func.get()));
            ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:510: +3, including nesting penalty of 2, nesting level increased to 3

            RETURN_IF_ERROR(_context->minmax_func->merge(wrapper->_context->minmax_func.get()));
            ^

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

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

be/src/exprs/runtime_filter.cpp:514: +2, including nesting penalty of 1, nesting level increased to 2

            RETURN_IF_ERROR(
            ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:514: +3, including nesting penalty of 2, nesting level increased to 3

            RETURN_IF_ERROR(
            ^

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

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

be/src/exprs/runtime_filter.cpp:522: +2, including nesting penalty of 1, nesting level increased to 2

            if (other_filter_type == RuntimeFilterType::IN_OR_BLOOM_FILTER) {
            ^

be/src/exprs/runtime_filter.cpp:526: +2, including nesting penalty of 1, nesting level increased to 2

            if (real_filter_type == RuntimeFilterType::IN_FILTER) {
            ^

be/src/exprs/runtime_filter.cpp:530: +3, including nesting penalty of 2, nesting level increased to 3

                if (other_filter_type == RuntimeFilterType::IN_FILTER) {
                ^

be/src/exprs/runtime_filter.cpp:532: +4, including nesting penalty of 3, nesting level increased to 4

                    if (_max_in_num >= 0 && _context->hybrid_set->size() >= _max_in_num) {
                    ^

be/src/exprs/runtime_filter.cpp:534: +5, including nesting penalty of 4, nesting level increased to 5

                        RETURN_IF_ERROR(_context->bloom_filter_func->init_with_fixed_length());
                        ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:534: +6, including nesting penalty of 5, nesting level increased to 6

                        RETURN_IF_ERROR(_context->bloom_filter_func->init_with_fixed_length());
                        ^

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

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

be/src/exprs/runtime_filter.cpp:535: +5, including nesting penalty of 4, nesting level increased to 5

                        RETURN_IF_ERROR(change_to_bloom_filter());
                        ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:535: +6, including nesting penalty of 5, nesting level increased to 6

                        RETURN_IF_ERROR(change_to_bloom_filter());
                        ^

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

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

be/src/exprs/runtime_filter.cpp:537: +1, nesting level increased to 3

                } else {
                  ^

be/src/exprs/runtime_filter.cpp:540: +4, including nesting penalty of 3, nesting level increased to 4

                    RETURN_IF_ERROR(change_to_bloom_filter());
                    ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:540: +5, including nesting penalty of 4, nesting level increased to 5

                    RETURN_IF_ERROR(change_to_bloom_filter());
                    ^

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

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

be/src/exprs/runtime_filter.cpp:542: +1, nesting level increased to 2

            } else {
              ^

be/src/exprs/runtime_filter.cpp:543: +3, including nesting penalty of 2, nesting level increased to 3

                if (other_filter_type == RuntimeFilterType::IN_FILTER) {
                ^

be/src/exprs/runtime_filter.cpp:546: +1, nesting level increased to 3

                } else {
                  ^

be/src/exprs/runtime_filter.cpp:548: +4, including nesting penalty of 3, nesting level increased to 4

                    RETURN_IF_ERROR(_context->bloom_filter_func->merge(
                    ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:548: +5, including nesting penalty of 4, nesting level increased to 5

                    RETURN_IF_ERROR(_context->bloom_filter_func->merge(
                    ^

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

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

@BiteTheDDDDt
Copy link
Contributor Author

run buildall

@BiteTheDDDDt
Copy link
Contributor Author

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

std::vector<vectorized::VRuntimeFilterPtr>& push_exprs,
const TExpr& probe_expr);

Status merge(const RuntimePredicateWrapper* wrapper) {
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 'merge' has cognitive complexity of 76 (threshold 50) [readability-function-cognitive-complexity]

    Status merge(const RuntimePredicateWrapper* wrapper) {
           ^
Additional context

be/src/exprs/runtime_filter.cpp:474: +1, including nesting penalty of 0, nesting level increased to 1

        if (wrapper->is_ignored()) {
        ^

be/src/exprs/runtime_filter.cpp:480: +1

                _filter_type == RuntimeFilterType::IN_OR_BLOOM_FILTER &&
                                                                      ^

be/src/exprs/runtime_filter.cpp:485: +1

        bool can_not_merge_other = _filter_type != RuntimeFilterType::IN_OR_BLOOM_FILTER &&
                                                                                         ^

be/src/exprs/runtime_filter.cpp:488: +1

        CHECK(!can_not_merge_in_or_bloom && !can_not_merge_other)
                                         ^

be/src/exprs/runtime_filter.cpp:493: +1, including nesting penalty of 0, nesting level increased to 1

        switch (_filter_type) {
        ^

be/src/exprs/runtime_filter.cpp:495: +2, including nesting penalty of 1, nesting level increased to 2

            if (!_context->hybrid_set) {
            ^

be/src/exprs/runtime_filter.cpp:500: +2, including nesting penalty of 1, nesting level increased to 2

            if (_max_in_num >= 0 && _context->hybrid_set->size() >= _max_in_num) {
            ^

be/src/exprs/runtime_filter.cpp:510: +2, including nesting penalty of 1, nesting level increased to 2

            RETURN_IF_ERROR(_context->minmax_func->merge(wrapper->_context->minmax_func.get()));
            ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:510: +3, including nesting penalty of 2, nesting level increased to 3

            RETURN_IF_ERROR(_context->minmax_func->merge(wrapper->_context->minmax_func.get()));
            ^

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

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

be/src/exprs/runtime_filter.cpp:514: +2, including nesting penalty of 1, nesting level increased to 2

            RETURN_IF_ERROR(
            ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:514: +3, including nesting penalty of 2, nesting level increased to 3

            RETURN_IF_ERROR(
            ^

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

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

be/src/exprs/runtime_filter.cpp:522: +2, including nesting penalty of 1, nesting level increased to 2

            if (other_filter_type == RuntimeFilterType::IN_OR_BLOOM_FILTER) {
            ^

be/src/exprs/runtime_filter.cpp:526: +2, including nesting penalty of 1, nesting level increased to 2

            if (real_filter_type == RuntimeFilterType::IN_FILTER) {
            ^

be/src/exprs/runtime_filter.cpp:530: +3, including nesting penalty of 2, nesting level increased to 3

                if (other_filter_type == RuntimeFilterType::IN_FILTER) {
                ^

be/src/exprs/runtime_filter.cpp:532: +4, including nesting penalty of 3, nesting level increased to 4

                    if (_max_in_num >= 0 && _context->hybrid_set->size() >= _max_in_num) {
                    ^

be/src/exprs/runtime_filter.cpp:534: +5, including nesting penalty of 4, nesting level increased to 5

                        RETURN_IF_ERROR(_context->bloom_filter_func->init_with_fixed_length());
                        ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:534: +6, including nesting penalty of 5, nesting level increased to 6

                        RETURN_IF_ERROR(_context->bloom_filter_func->init_with_fixed_length());
                        ^

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

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

be/src/exprs/runtime_filter.cpp:535: +5, including nesting penalty of 4, nesting level increased to 5

                        RETURN_IF_ERROR(change_to_bloom_filter());
                        ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:535: +6, including nesting penalty of 5, nesting level increased to 6

                        RETURN_IF_ERROR(change_to_bloom_filter());
                        ^

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

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

be/src/exprs/runtime_filter.cpp:537: +1, nesting level increased to 3

                } else {
                  ^

be/src/exprs/runtime_filter.cpp:540: +4, including nesting penalty of 3, nesting level increased to 4

                    RETURN_IF_ERROR(change_to_bloom_filter());
                    ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:540: +5, including nesting penalty of 4, nesting level increased to 5

                    RETURN_IF_ERROR(change_to_bloom_filter());
                    ^

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

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

be/src/exprs/runtime_filter.cpp:542: +1, nesting level increased to 2

            } else {
              ^

be/src/exprs/runtime_filter.cpp:543: +3, including nesting penalty of 2, nesting level increased to 3

                if (other_filter_type == RuntimeFilterType::IN_FILTER) {
                ^

be/src/exprs/runtime_filter.cpp:546: +1, nesting level increased to 3

                } else {
                  ^

be/src/exprs/runtime_filter.cpp:548: +4, including nesting penalty of 3, nesting level increased to 4

                    RETURN_IF_ERROR(_context->bloom_filter_func->merge(
                    ^

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

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:548: +5, including nesting penalty of 4, nesting level increased to 5

                    RETURN_IF_ERROR(_context->bloom_filter_func->merge(
                    ^

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

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

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Oct 11, 2024
@github-actions
Copy link
Contributor

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@BiteTheDDDDt BiteTheDDDDt merged commit 8be40e2 into apache:master Oct 11, 2024
Gabriel39 pushed a commit to Gabriel39/incubator-doris that referenced this pull request Oct 16, 2024
…ly (apache#41667)

send ignored rf when hash join build closed early to avoid runtime
filter sync/merge error
Follow-up : apache#41292
Gabriel39 added a commit that referenced this pull request Oct 16, 2024
 #41751 (#41927)

## Proposed changes

pick #41292 #41350 #41589 #41628 #41743 #41601 #41667 #41751

<!--Describe your changes.-->

---------

Co-authored-by: Pxl <pxl290@qq.com>
Gabriel39 pushed a commit to Gabriel39/incubator-doris that referenced this pull request Oct 17, 2024
…ly (apache#41667)

## Proposed changes
send ignored rf when hash join build closed early to avoid runtime
filter sync/merge error
Follow-up : apache#41292
BiteTheDDDDt added a commit that referenced this pull request Nov 4, 2024
)

In ##41667 we support ignore filter
partitial, however, when sync_filter_size is turned off, the final
filter may only contain part of the data because some filters are judged
to be 'in'.
For example:
there are rf001(2 instances, in_or_bloom),rf000(2 instances, min_max),
instance_1 has 1e8 row and rf001 change to bloom, the rf000 will not
ignored
instance_2 has 1 row and rf001 change to in, the rf000 will ignored
finally, rf000 applied and make wrong result
github-actions bot pushed a commit that referenced this pull request Nov 4, 2024
)

In ##41667 we support ignore filter
partitial, however, when sync_filter_size is turned off, the final
filter may only contain part of the data because some filters are judged
to be 'in'.
For example:
there are rf001(2 instances, in_or_bloom),rf000(2 instances, min_max),
instance_1 has 1e8 row and rf001 change to bloom, the rf000 will not
ignored
instance_2 has 1 row and rf001 change to in, the rf000 will ignored
finally, rf000 applied and make wrong result
BiteTheDDDDt added a commit that referenced this pull request Nov 27, 2024
)

In ##41667 we support ignore filter
partitial, however, when sync_filter_size is turned off, the final
filter may only contain part of the data because some filters are judged
to be 'in'.
For example:
there are rf001(2 instances, in_or_bloom),rf000(2 instances, min_max),
instance_1 has 1e8 row and rf001 change to bloom, the rf000 will not
ignored
instance_2 has 1 row and rf001 change to in, the rf000 will ignored
finally, rf000 applied and make wrong result
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by one committer. dev/2.1.7-merged dev/3.0.3-merged reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants