This is an automated email from the ASF dual-hosted git repository. kxiao pushed a commit to branch branch-2.0 in repository https://gitbox.apache.org/repos/asf/doris.git
commit 90edaead391d7f9128f19a64b4a90771934f24a4 Author: airborne12 <[email protected]> AuthorDate: Thu Jul 6 11:52:59 2023 +0800 [Enhancement](inverted index) make InvertedIndexReader shared_from_this (#21381) This PR proposes several changes to improve code safety and readability by replacing raw pointers with smart pointers in several places. use enable_factory_creator in InvertedIndexIterator and InvertedIndexReader, remove explicit new constructor. make InvertedIndexReader shared_from_this, it may desctruct when InvertedIndexIterator use it. --- be/src/olap/rowset/segment_v2/column_reader.cpp | 14 +++--- be/src/olap/rowset/segment_v2/column_reader.h | 4 +- .../rowset/segment_v2/inverted_index_reader.cpp | 17 ++++--- .../olap/rowset/segment_v2/inverted_index_reader.h | 57 ++++++++++++++-------- be/src/olap/rowset/segment_v2/segment.cpp | 4 +- be/src/olap/rowset/segment_v2/segment_iterator.cpp | 1 + 6 files changed, 58 insertions(+), 39 deletions(-) diff --git a/be/src/olap/rowset/segment_v2/column_reader.cpp b/be/src/olap/rowset/segment_v2/column_reader.cpp index 5c3e67e0a2..a27d999169 100644 --- a/be/src/olap/rowset/segment_v2/column_reader.cpp +++ b/be/src/olap/rowset/segment_v2/column_reader.cpp @@ -233,7 +233,7 @@ Status ColumnReader::new_bitmap_index_iterator(BitmapIndexIterator** iterator) { Status ColumnReader::new_inverted_index_iterator(const TabletIndex* index_meta, OlapReaderStatistics* stats, - InvertedIndexIterator** iterator) { + std::unique_ptr<InvertedIndexIterator>* iterator) { RETURN_IF_ERROR(_ensure_inverted_index_loaded(index_meta)); if (_inverted_index) { RETURN_IF_ERROR(_inverted_index->new_iterator(stats, iterator)); @@ -505,16 +505,16 @@ Status ColumnReader::_load_inverted_index_index(const TabletIndex* index_meta) { if (is_string_type(type)) { if (parser_type != InvertedIndexParserType::PARSER_NONE) { - _inverted_index.reset(new FullTextIndexReader( - _file_reader->fs(), _file_reader->path().native(), index_meta)); + _inverted_index = FullTextIndexReader::create_shared( + _file_reader->fs(), _file_reader->path().native(), index_meta); return Status::OK(); } else { - _inverted_index.reset(new StringTypeInvertedIndexReader( - _file_reader->fs(), _file_reader->path().native(), index_meta)); + _inverted_index = StringTypeInvertedIndexReader::create_shared( + _file_reader->fs(), _file_reader->path().native(), index_meta); } } else if (is_numeric_type(type)) { - _inverted_index.reset( - new BkdIndexReader(_file_reader->fs(), _file_reader->path().native(), index_meta)); + _inverted_index = BkdIndexReader::create_shared(_file_reader->fs(), + _file_reader->path().native(), index_meta); } else { _inverted_index.reset(); } diff --git a/be/src/olap/rowset/segment_v2/column_reader.h b/be/src/olap/rowset/segment_v2/column_reader.h index 6cb9794b3b..a6d23ac950 100644 --- a/be/src/olap/rowset/segment_v2/column_reader.h +++ b/be/src/olap/rowset/segment_v2/column_reader.h @@ -120,7 +120,7 @@ public: Status new_bitmap_index_iterator(BitmapIndexIterator** iterator); Status new_inverted_index_iterator(const TabletIndex* index_meta, OlapReaderStatistics* stats, - InvertedIndexIterator** iterator); + std::unique_ptr<InvertedIndexIterator>* iterator); // Seek to the first entry in the column. Status seek_to_first(OrdinalPageIndexIterator* iter); @@ -237,7 +237,7 @@ private: std::unique_ptr<ZoneMapIndexReader> _zone_map_index; std::unique_ptr<OrdinalIndexReader> _ordinal_index; std::unique_ptr<BitmapIndexReader> _bitmap_index; - std::unique_ptr<InvertedIndexReader> _inverted_index; + std::shared_ptr<InvertedIndexReader> _inverted_index; std::unique_ptr<BloomFilterIndexReader> _bloom_filter_index; DorisCallOnce<Status> _load_zone_map_index_once; DorisCallOnce<Status> _load_ordinal_index_once; diff --git a/be/src/olap/rowset/segment_v2/inverted_index_reader.cpp b/be/src/olap/rowset/segment_v2/inverted_index_reader.cpp index fae74fc883..8e06ff1c8c 100644 --- a/be/src/olap/rowset/segment_v2/inverted_index_reader.cpp +++ b/be/src/olap/rowset/segment_v2/inverted_index_reader.cpp @@ -213,8 +213,8 @@ Status InvertedIndexReader::read_null_bitmap(InvertedIndexQueryCacheHandle* cach } Status FullTextIndexReader::new_iterator(OlapReaderStatistics* stats, - InvertedIndexIterator** iterator) { - *iterator = new InvertedIndexIterator(stats, this); + std::unique_ptr<InvertedIndexIterator>* iterator) { + *iterator = InvertedIndexIterator::create_unique(stats, shared_from_this()); return Status::OK(); } @@ -404,9 +404,9 @@ InvertedIndexReaderType FullTextIndexReader::type() { return InvertedIndexReaderType::FULLTEXT; } -Status StringTypeInvertedIndexReader::new_iterator(OlapReaderStatistics* stats, - InvertedIndexIterator** iterator) { - *iterator = new InvertedIndexIterator(stats, this); +Status StringTypeInvertedIndexReader::new_iterator( + OlapReaderStatistics* stats, std::unique_ptr<InvertedIndexIterator>* iterator) { + *iterator = InvertedIndexIterator::create_unique(stats, shared_from_this()); return Status::OK(); } @@ -543,13 +543,14 @@ BkdIndexReader::BkdIndexReader(io::FileSystemSPtr fs, const std::string& path, LOG(WARNING) << "bkd index: " << index_file.string() << " not exist."; return; } - _compoundReader = new DorisCompoundReader( + _compoundReader = std::make_unique<DorisCompoundReader>( DorisCompoundDirectory::getDirectory(fs, index_dir.c_str()), index_file_name.c_str(), config::inverted_index_read_buffer_size); } -Status BkdIndexReader::new_iterator(OlapReaderStatistics* stats, InvertedIndexIterator** iterator) { - *iterator = new InvertedIndexIterator(stats, this); +Status BkdIndexReader::new_iterator(OlapReaderStatistics* stats, + std::unique_ptr<InvertedIndexIterator>* iterator) { + *iterator = InvertedIndexIterator::create_unique(stats, shared_from_this()); return Status::OK(); } diff --git a/be/src/olap/rowset/segment_v2/inverted_index_reader.h b/be/src/olap/rowset/segment_v2/inverted_index_reader.h index 1b30286a15..d74ada9d8e 100644 --- a/be/src/olap/rowset/segment_v2/inverted_index_reader.h +++ b/be/src/olap/rowset/segment_v2/inverted_index_reader.h @@ -64,15 +64,16 @@ enum class InvertedIndexReaderType { BKD = 2, }; -class InvertedIndexReader { +class InvertedIndexReader : public std::enable_shared_from_this<InvertedIndexReader> { public: explicit InvertedIndexReader(io::FileSystemSPtr fs, const std::string& path, const TabletIndex* index_meta) - : _fs(std::move(fs)), _path(path), _index_meta(*index_meta) {} + : _fs(fs), _path(path), _index_meta(*index_meta) {} virtual ~InvertedIndexReader() = default; // create a new column iterator. Client should delete returned iterator - virtual Status new_iterator(OlapReaderStatistics* stats, InvertedIndexIterator** iterator) = 0; + virtual Status new_iterator(OlapReaderStatistics* stats, + std::unique_ptr<InvertedIndexIterator>* iterator) = 0; virtual Status query(OlapReaderStatistics* stats, const std::string& column_name, const void* query_value, InvertedIndexQueryType query_type, roaring::Roaring* bit_map) = 0; @@ -86,9 +87,9 @@ public: virtual InvertedIndexReaderType type() = 0; bool indexExists(io::Path& index_file_path); - uint32_t get_index_id() const { return _index_meta.index_id(); } + [[nodiscard]] uint32_t get_index_id() const { return _index_meta.index_id(); } - const std::map<string, string>& get_index_properties() const { + [[nodiscard]] const std::map<string, string>& get_index_properties() const { return _index_meta.properties(); } @@ -103,18 +104,21 @@ protected: bool _is_match_query(InvertedIndexQueryType query_type); friend class InvertedIndexIterator; io::FileSystemSPtr _fs; - std::string _path; + const std::string& _path; TabletIndex _index_meta; }; class FullTextIndexReader : public InvertedIndexReader { + ENABLE_FACTORY_CREATOR(FullTextIndexReader); + public: explicit FullTextIndexReader(io::FileSystemSPtr fs, const std::string& path, const TabletIndex* index_meta) - : InvertedIndexReader(std::move(fs), path, index_meta) {} + : InvertedIndexReader(fs, path, index_meta) {} ~FullTextIndexReader() override = default; - Status new_iterator(OlapReaderStatistics* stats, InvertedIndexIterator** iterator) override; + Status new_iterator(OlapReaderStatistics* stats, + std::unique_ptr<InvertedIndexIterator>* iterator) override; Status query(OlapReaderStatistics* stats, const std::string& column_name, const void* query_value, InvertedIndexQueryType query_type, roaring::Roaring* bit_map) override; @@ -128,13 +132,16 @@ public: }; class StringTypeInvertedIndexReader : public InvertedIndexReader { + ENABLE_FACTORY_CREATOR(StringTypeInvertedIndexReader); + public: explicit StringTypeInvertedIndexReader(io::FileSystemSPtr fs, const std::string& path, const TabletIndex* index_meta) - : InvertedIndexReader(std::move(fs), path, index_meta) {} + : InvertedIndexReader(fs, path, index_meta) {} ~StringTypeInvertedIndexReader() override = default; - Status new_iterator(OlapReaderStatistics* stats, InvertedIndexIterator** iterator) override; + Status new_iterator(OlapReaderStatistics* stats, + std::unique_ptr<InvertedIndexIterator>* iterator) override; Status query(OlapReaderStatistics* stats, const std::string& column_name, const void* query_value, InvertedIndexQueryType query_type, roaring::Roaring* bit_map) override; @@ -181,18 +188,28 @@ public: }; class BkdIndexReader : public InvertedIndexReader { + ENABLE_FACTORY_CREATOR(BkdIndexReader); + public: explicit BkdIndexReader(io::FileSystemSPtr fs, const std::string& path, const TabletIndex* index_meta); ~BkdIndexReader() override { if (_compoundReader != nullptr) { - _compoundReader->close(); - delete _compoundReader; - _compoundReader = nullptr; + try { + _compoundReader->close(); + } catch (const CLuceneError& e) { + // Handle exception, e.g., log it, but don't rethrow. + LOG(ERROR) << "Exception caught in BkdIndexReader destructor: " << e.what() + << std::endl; + } catch (...) { + // Handle all other exceptions, but don't rethrow. + LOG(ERROR) << "Unknown exception caught in BkdIndexReader destructor." << std::endl; + } } } - Status new_iterator(OlapReaderStatistics* stats, InvertedIndexIterator** iterator) override; + Status new_iterator(OlapReaderStatistics* stats, + std::unique_ptr<InvertedIndexIterator>* iterator) override; Status query(OlapReaderStatistics* stats, const std::string& column_name, const void* query_value, InvertedIndexQueryType query_type, @@ -211,12 +228,14 @@ public: private: const TypeInfo* _type_info {}; const KeyCoder* _value_key_coder {}; - DorisCompoundReader* _compoundReader; + std::unique_ptr<DorisCompoundReader> _compoundReader; }; class InvertedIndexIterator { + ENABLE_FACTORY_CREATOR(InvertedIndexIterator); + public: - InvertedIndexIterator(OlapReaderStatistics* stats, InvertedIndexReader* reader) + InvertedIndexIterator(OlapReaderStatistics* stats, std::shared_ptr<InvertedIndexReader> reader) : _stats(stats), _reader(reader) {} Status read_from_inverted_index(const std::string& column_name, const void* query_value, @@ -230,12 +249,12 @@ public: return _reader->read_null_bitmap(cache_handle, dir); } - InvertedIndexReaderType get_inverted_index_reader_type() const; - const std::map<string, string>& get_index_properties() const; + [[nodiscard]] InvertedIndexReaderType get_inverted_index_reader_type() const; + [[nodiscard]] const std::map<string, string>& get_index_properties() const; private: OlapReaderStatistics* _stats; - InvertedIndexReader* _reader; + std::shared_ptr<InvertedIndexReader> _reader; }; } // namespace segment_v2 diff --git a/be/src/olap/rowset/segment_v2/segment.cpp b/be/src/olap/rowset/segment_v2/segment.cpp index 70ac2c108e..598a14f1b0 100644 --- a/be/src/olap/rowset/segment_v2/segment.cpp +++ b/be/src/olap/rowset/segment_v2/segment.cpp @@ -346,10 +346,8 @@ Status Segment::new_inverted_index_iterator(const TabletColumn& tablet_column, std::unique_ptr<InvertedIndexIterator>* iter) { auto col_unique_id = tablet_column.unique_id(); if (_column_readers.count(col_unique_id) > 0 && index_meta) { - InvertedIndexIterator* it; RETURN_IF_ERROR(_column_readers.at(col_unique_id) - ->new_inverted_index_iterator(index_meta, stats, &it)); - iter->reset(it); + ->new_inverted_index_iterator(index_meta, stats, iter)); return Status::OK(); } return Status::OK(); diff --git a/be/src/olap/rowset/segment_v2/segment_iterator.cpp b/be/src/olap/rowset/segment_v2/segment_iterator.cpp index eaa3102d0b..2cc1b58a56 100644 --- a/be/src/olap/rowset/segment_v2/segment_iterator.cpp +++ b/be/src/olap/rowset/segment_v2/segment_iterator.cpp @@ -820,6 +820,7 @@ std::string SegmentIterator::_gen_predicate_result_sign(ColumnPredicateInfo* pre bool SegmentIterator::_column_has_fulltext_index(int32_t unique_id) { bool has_fulltext_index = + _inverted_index_iterators.count(unique_id) > 0 && _inverted_index_iterators[unique_id] != nullptr && _inverted_index_iterators[unique_id]->get_inverted_index_reader_type() == InvertedIndexReaderType::FULLTEXT; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
