Skip to content

[branch-2.1](memory) Fix memory tracker destructor deadlock#33504

Closed
xinyiZzz wants to merge 19 commits intoapache:branch-2.1from
xinyiZzz:branch-2.1_20240410_fix_memtracker_weakptr
Closed

[branch-2.1](memory) Fix memory tracker destructor deadlock#33504
xinyiZzz wants to merge 19 commits intoapache:branch-2.1from
xinyiZzz:branch-2.1_20240410_fix_memtracker_weakptr

Conversation

@xinyiZzz
Copy link
Contributor

Proposed changes

pick #33497

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

BiteTheDDDDt and others added 19 commits April 10, 2024 16:25
…e to init bloom runtime filter (apache#32180)

support sync join node build side's size to init bloom runtime filter
…lumn (apache#31824)

Problem: An error is encountered when executing a schema change on a unique table to add a column with a complex type, such as bitmap, as documented in apache#31365

Reason: The error arises because the schema change logic erroneously applies an aggregation check for new columns across all table types, demanding an explicit aggregation type declaration. However, unique and duplicate tables inherently assume default aggregation types for newly added columns, leading to this misstep.

Solution: The schema change process for introducing new columns needs to distinguish between table types accurately. For unique and duplicate tables, it should automatically assign the appropriate aggregation type, which, for the purpose of smooth integration with subsequent processes, should be set to NONE.
…pache#33205)

fix legacy planner bug when insert overwrite non-partition table.
…33124)

support complex type and ip/jsonb in DataTypeSerDe::write_column_to_pb/read_column_from_pb function
apache#33349) (apache#33486)

* [bug](mem_tracker) fix mem_tracker dcheck failed as not used correctly
@doris-robot
Copy link

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

Since 2024-03-18, the Document has been moved to doris-website.
See Doris Document.

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 73 (threshold 50) [readability-function-cognitive-complexity]

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

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

        if (is_ignored() || wrapper->is_ignored()) {
        ^

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

        if (is_ignored() || wrapper->is_ignored()) {
                         ^

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

                _filter_type == RuntimeFilterType::IN_OR_BLOOM_FILTER &&
                                                                      ^

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

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

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

        CHECK(!can_not_merge_in_or_bloom && !can_not_merge_other)
                                         ^

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

        switch (_filter_type) {
        ^

be/src/exprs/runtime_filter.cpp:486: +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:496: +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:538: expanded from macro 'RETURN_IF_ERROR'

    do {                                \
    ^

be/src/exprs/runtime_filter.cpp:496: +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:540: expanded from macro 'RETURN_IF_ERROR'

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

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

            RETURN_IF_ERROR(
            ^

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

    do {                                \
    ^

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

            RETURN_IF_ERROR(
            ^

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

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

be/src/exprs/runtime_filter.cpp:508: +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:512: +2, including nesting penalty of 1, nesting level increased to 2

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

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

                if (other_filter_type == RuntimeFilterType::IN_FILTER) { // in merge in
                ^

be/src/exprs/runtime_filter.cpp:515: +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:519: +5, including nesting penalty of 4, nesting level increased to 5

                        RETURN_IF_ERROR(change_to_bloom_filter(true));
                        ^

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

    do {                                \
    ^

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

                        RETURN_IF_ERROR(change_to_bloom_filter(true));
                        ^

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

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

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

                } else {
                  ^

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

                    RETURN_IF_ERROR(change_to_bloom_filter(false));
                    ^

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

    do {                                \
    ^

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

                    RETURN_IF_ERROR(change_to_bloom_filter(false));
                    ^

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

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

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

                    RETURN_IF_ERROR(_context->bloom_filter_func->merge(
                    ^

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

    do {                                \
    ^

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

                    RETURN_IF_ERROR(_context->bloom_filter_func->merge(
                    ^

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

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

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

            } else {
              ^

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

                if (other_filter_type == RuntimeFilterType::IN_FILTER) { // bloom filter merge in
                ^

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

                } else {
                  ^

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

                    RETURN_IF_ERROR(_context->bloom_filter_func->merge(
                    ^

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

    do {                                \
    ^

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

                    RETURN_IF_ERROR(_context->bloom_filter_func->merge(
                    ^

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

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

auto ignore_remote_filter = [](IRuntimeFilter* runtime_filter, std::string& msg) {
runtime_filter->set_ignored(msg);
RETURN_IF_ERROR(runtime_filter->publish());
Status send_filter_size(RuntimeState* state, uint64_t hash_table_size, bool publish_local,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'send_filter_size' can be made static [readability-convert-member-functions-to-static]

Suggested change
Status send_filter_size(RuntimeState* state, uint64_t hash_table_size, bool publish_local,
static Status send_filter_size(RuntimeState* state, uint64_t hash_table_size, bool publish_local,

if (over_max_in_num &&
runtime_filter->type() == RuntimeFilterType::IN_OR_BLOOM_FILTER) {
RETURN_IF_ERROR(runtime_filter->change_to_bloom_filter());
Status ignore_filters(RuntimeState* state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'ignore_filters' can be made static [readability-convert-member-functions-to-static]

Suggested change
Status ignore_filters(RuntimeState* state) {
static Status ignore_filters(RuntimeState* state) {

(runtime_filter->type() == RuntimeFilterType::IN_OR_BLOOM_FILTER &&
!over_max_in_num)) {
has_in_filter[runtime_filter->expr_order()] = true;
Status init_filters(RuntimeState* state, uint64_t local_hash_table_size) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'init_filters' can be made static [readability-convert-member-functions-to-static]

Suggested change
Status init_filters(RuntimeState* state, uint64_t local_hash_table_size) {
static Status init_filters(RuntimeState* state, uint64_t local_hash_table_size) {

return Status::OK();
}

Status HashJoinBuildSinkLocalState::close(RuntimeState* state, Status exec_status) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'close' can be made const [readability-make-member-function-const]

be/src/pipeline/exec/hashjoin_build_sink.h:74:

-     Status close(RuntimeState* state, Status exec_status) override;
+     Status close(RuntimeState* state, Status exec_status) const override;
Suggested change
Status HashJoinBuildSinkLocalState::close(RuntimeState* state, Status exec_status) {
Status HashJoinBuildSinkLocalState::close(RuntimeState* state, Status exec_status) const {

parser.getWriter().getOutput()->getSize());
return Status::OK();
}
Status DataTypeJsonbSerDe::write_column_to_pb(const IColumn& column, PValues& result, int start,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'write_column_to_pb' can be made static [readability-convert-member-functions-to-static]

be/src/vec/data_types/serde/data_type_jsonb_serde.cpp:242:

-                                               int end) const {
+                                               int end) {

be/src/vec/data_types/serde/data_type_jsonb_serde.h:67:

-     Status write_column_to_pb(const IColumn& column, PValues& result, int start,
-                               int end) const override;
+     static Status write_column_to_pb(const IColumn& column, PValues& result, int start,
+                               int end) override;

return Status::OK();
}

Status DataTypeJsonbSerDe::read_column_from_pb(IColumn& column, const PValues& arg) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'read_column_from_pb' can be made static [readability-convert-member-functions-to-static]

Suggested change
Status DataTypeJsonbSerDe::read_column_from_pb(IColumn& column, const PValues& arg) const {
Status DataTypeJsonbSerDe::read_column_from_pb(IColumn& column, const PValues& arg) {

be/src/vec/data_types/serde/data_type_jsonb_serde.h:69:

-     Status read_column_from_pb(IColumn& column, const PValues& arg) const override;
+     static Status read_column_from_pb(IColumn& column, const PValues& arg) override;

return Status::OK();
}

Status VExpr::check_expr_output_type(const VExprContextSPtrs& ctxs,
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'check_expr_output_type' can be made static [readability-convert-member-functions-to-static]

Suggested change
Status VExpr::check_expr_output_type(const VExprContextSPtrs& ctxs,
static Status VExpr::check_expr_output_type(const VExprContextSPtrs& ctxs,

@@ -19,6 +19,7 @@
#include <gen_cpp/types.pb.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'gen_cpp/types.pb.h' file not found [clang-diagnostic-error]

#include <gen_cpp/types.pb.h>
         ^

if (!st.ok()) {
std::cerr << "pb_to_column error, maybe not impl it: " << st.msg() << " "
<< data_type->get_name() << std::endl;
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: redundant boolean literal in conditional return statement [readability-simplify-boolean-expr]

be/test/vec/data_types/serde/data_type_serde_pb_test.cpp:78:

-     if (!st.ok()) {
-         std::cerr << "pb_to_column error, maybe not impl it: " << st.msg() << " "
-                   << data_type->get_name() << std::endl;
-         return false;
-     }
-     return true;
+     return st.ok();

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.