[GitHub] [incubator-doris] HappenLee commented on a diff in pull request #10003: [Feature] [Vectorized] Some pre-refactorings or interface additions for schema change part2

[GitBox]

HappenLee commented on code in PR #10003:
URL: https://github.com/apache/incubator-doris/pull/10003#discussion_r892290286


##########
be/src/olap/schema_change.h:
##########
@@ -70,6 +59,8 @@ class RowBlockChanger {
     // delete handler for filtering data which use specified in DELETE_DATA
     const DeleteHandler* _delete_handler = nullptr;
 
+    DescriptorTbl _desc_tbl;

Review Comment:
   why add `_desc_tbl` seems only use in construct function?



##########
be/src/olap/schema_change.h:
##########
@@ -94,20 +85,62 @@ class SchemaChange {
     SchemaChange() : _filtered_rows(0), _merged_rows(0) {}
     virtual ~SchemaChange() = default;
 
-    virtual Status process(RowsetReaderSharedPtr rowset_reader, RowsetWriter* 
new_rowset_builder,
-                           TabletSharedPtr tablet, TabletSharedPtr 
base_tablet) = 0;
-
-    void add_filtered_rows(uint64_t filtered_rows) { _filtered_rows += 
filtered_rows; }
-
-    void add_merged_rows(uint64_t merged_rows) { _merged_rows += merged_rows; }
+    virtual Status process(RowsetReaderSharedPtr rowset_reader, RowsetWriter* 
rowset_writer,
+                           TabletSharedPtr new_tablet, TabletSharedPtr 
base_tablet) {
+        if (rowset_reader->rowset()->empty() || 
rowset_reader->rowset()->num_rows() == 0) {
+            RETURN_WITH_WARN_IF_ERROR(
+                    rowset_writer->flush(),
+                    Status::OLAPInternalError(OLAP_ERR_INPUT_PARAMETER_ERROR),
+                    fmt::format("create empty version for schema change 
failed. version= {}-{}",
+                                rowset_writer->version().first, 
rowset_writer->version().second));
+
+            return Status::OK();
+        }
+
+        _filtered_rows = 0;
+        _merged_rows = 0;
+
+        RETURN_IF_ERROR(_inner_process(rowset_reader, rowset_writer, 
new_tablet, base_tablet));
+        _add_filtered_rows(rowset_reader->filtered_rows());
+
+        // Check row num changes
+        if (config::row_nums_check) {

Review Comment:
   `if (config::row_nums_check && !_check_row_nums(rowset_reader, 
*rowset_writer))`



##########
be/src/olap/schema_change.cpp:
##########
@@ -807,22 +807,32 @@ bool 
RowBlockAllocator::is_memory_enough_for_sorting(size_t num_rows, size_t all
 
 RowBlockMerger::RowBlockMerger(TabletSharedPtr tablet) : _tablet(tablet) {}
 
-RowBlockMerger::~RowBlockMerger() {}
+RowBlockMerger::~RowBlockMerger() = default;
 
 bool RowBlockMerger::merge(const std::vector<RowBlock*>& row_block_arr, 
RowsetWriter* rowset_writer,
                            uint64_t* merged_rows) {
     uint64_t tmp_merged_rows = 0;
     RowCursor row_cursor;
     std::unique_ptr<MemPool> mem_pool(new MemPool("RowBlockMerger"));
     std::unique_ptr<ObjectPool> agg_object_pool(new ObjectPool());
+
+    auto merge_error = [&]() -> bool {
+        while (_heap.size() > 0) {

Review Comment:
   `!_heap.empty()` same in line 839



##########
be/src/olap/schema_change.cpp:
##########
@@ -917,37 +918,33 @@ void RowBlockMerger::_pop_heap() {
     return;

Review Comment:
   useless return



##########
be/src/olap/schema_change.cpp:
##########
@@ -92,19 +91,20 @@ class RowBlockMerger {
     std::priority_queue<MergeElement> _heap;
 };
 
-RowBlockChanger::RowBlockChanger(const TabletSchema& tablet_schema) {
+RowBlockChanger::RowBlockChanger(const TabletSchema& tablet_schema, 
DescriptorTbl desc_tbl)
+        : _desc_tbl(desc_tbl) {
     _schema_mapping.resize(tablet_schema.num_columns());
 }
 
 RowBlockChanger::RowBlockChanger(const TabletSchema& tablet_schema,
-                                 const DeleteHandler* delete_handler) {
+                                 const DeleteHandler* delete_handler, 
DescriptorTbl desc_tbl)
+        : _desc_tbl(desc_tbl) {
     _schema_mapping.resize(tablet_schema.num_columns());
     _delete_handler = delete_handler;
 }
 
 RowBlockChanger::~RowBlockChanger() {
-    SchemaMapping::iterator it = _schema_mapping.begin();
-    for (; it != _schema_mapping.end(); ++it) {
+    for (auto it = _schema_mapping.begin(); it != _schema_mapping.end(); ++it) 
{
         SAFE_DELETE(it->default_value);

Review Comment:
   recheck the logic? maybe we should do the work in `~ColumnMapping` and 
`RowBlockChanger::~RowBlockChanger() = default`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org