Skip to content

[bugfix 1.1.1] fix bug of vhash join build#10614

Merged
yiguolei merged 2 commits intoapache:masterfrom
jacktengg:fix-vhash-join-build-blocks
Jul 5, 2022
Merged

[bugfix 1.1.1] fix bug of vhash join build#10614
yiguolei merged 2 commits intoapache:masterfrom
jacktengg:fix-vhash-join-build-blocks

Conversation

@jacktengg
Copy link
Contributor

@jacktengg jacktengg commented Jul 5, 2022

Proposed changes

Issue Number: close #10595

Problem Summary:

When build hash table in HashJoinNode::_hash_table_build, build side data is collected as a vector of Blocks of size 4G at maximum, if it happens that the total size of build side data is exactly e.g. 4G, then an empty Block will be added to the vector:

    while (!eos) {
        block.clear_column_data();
        RETURN_IF_CANCELLED(state);

        RETURN_IF_ERROR(child(1)->get_next(state, &block, &eos));
        _hash_table_mem_tracker->consume(block.allocated_bytes());
        _mem_used += block.allocated_bytes();

        if (block.rows() != 0) {
            mutable_block.merge(block);
        }

        // make one block for each 4 gigabytes
        constexpr static auto BUILD_BLOCK_MAX_SIZE = 4 * 1024UL * 1024UL * 1024UL;
        if (UNLIKELY(_mem_used - last_mem_used > BUILD_BLOCK_MAX_SIZE)) {
            _build_blocks.emplace_back(mutable_block.to_block());
            // TODO:: Rethink may we should do the proess after we recevie all build blocks ?
            // which is better.
            RETURN_IF_ERROR(_process_build_block(state, _build_blocks[index], index));

            mutable_block = MutableBlock();
            ++index;
            last_mem_used = _mem_used;
        }
    }

    _build_blocks.emplace_back(mutable_block.to_block());

Later when building RF in RuntimeFilterSlotsBase::insert, it will try to get a column from an empty Block, which causes the problem.

Also fixed a potential bug: when build side data exceed 4G * 128, the Blocks added previously into the vector may be relocated, which will invalidate the Block address used as map key in td::unordered_map<const Block*, std::vector<int>> _inserted_rows;.

Checklist(Required)

  1. Does it affect the original behavior: (Yes/No/I Don't know)
  2. Has unit tests been added: (Yes/No/No Need)
  3. Has document been added or modified: (Yes/No/No Need)
  4. Does it need to update dependencies: (Yes/No)
  5. Are there any changes that cannot be rolled back: (Yes/No)

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...

#include "vec/exprs/vexpr_context.h"
#include "vec/utils/template_helpers.hpp"
#include "vec/utils/util.hpp"
#include "gutil/strings/substitute.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

should use BE code formater to format the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@dataroaring dataroaring left a comment

Choose a reason for hiding this comment

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

It is better to explain how the bug happens.

@jacktengg
Copy link
Contributor Author

Comment is added in Problem Summary.

Copy link
Contributor

@yiguolei yiguolei left a comment

Choose a reason for hiding this comment

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

LGTM

@yiguolei yiguolei merged commit 3e87960 into apache:master Jul 5, 2022
jacktengg added a commit to jacktengg/incubator-doris that referenced this pull request Jul 18, 2022
Merge fix of apache#10614 to branch dev-1.1.1.
yiguolei pushed a commit that referenced this pull request Jul 18, 2022
@yiguolei yiguolei changed the title [bugfix] fix bug of vhash join build [bugfix 1.1.1] fix bug of vhash join build Jul 18, 2022
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.

[Bug] be coredump when building RF

4 participants