[GitHub] [incubator-doris] morningman commented on a change in pull request #5491: [BUG] Fix Memory Leak in SchemaChange And Fix some DCHECK error

[GitBox <gi...@apache.org>]

morningman commented on a change in pull request #5491:
URL: https://github.com/apache/incubator-doris/pull/5491#discussion_r592385567



##########
File path: be/src/exec/analytic_eval_node.cpp
##########
@@ -753,7 +753,8 @@ Status AnalyticEvalNode::get_next_output_batch(RuntimeState* state, RowBatch* ou
         // CopyRow works as expected: input_batch tuples form a prefix of output_batch
         // tuples.
         TupleRow* dest = output_batch->get_row(output_batch->add_row());
-        input_batch.copy_row(input_batch.get_row(i), dest);
+        input_batch.get_row(i)->deep_copy(dest, child(0)->row_desc().tuple_descriptors(),

Review comment:
       What is different between copy_row and deep_copy?
   modify the comment explain why

##########
File path: be/src/exec/olap_utils.h
##########
@@ -207,6 +207,9 @@ inline SQLFilterOp to_olap_filter_type(TExprOpcode::type type, bool opposite) {
     case TExprOpcode::NE:
         return opposite ? FILTER_IN : FILTER_NOT_IN;
 
+    case TExprOpcode::EQ_FOR_NULL:

Review comment:
       Can the storage engine handle `EQ_FOR_NULL` correctly with `FILTER_IN`?

##########
File path: be/src/olap/aggregate_func.h
##########
@@ -461,10 +461,9 @@ struct AggregateFuncTraits<OLAP_FIELD_AGGREGATION_HLL_UNION, OLAP_FIELD_TYPE_HLL
         // we use zero size represent this slice is a agg object
         dst_slice->size = 0;
         auto* hll = new HyperLogLog(*src_slice);
+        
         dst_slice->data = reinterpret_cast<char*>(hll);
 
-        mem_pool->mem_tracker()->Consume(sizeof(HyperLogLog));

Review comment:
       Why remove this?

##########
File path: be/src/runtime/tablets_channel.cpp
##########
@@ -260,7 +260,6 @@ Status TabletsChannel::cancel() {
     for (auto& it : _tablet_writers) {
         it.second->cancel();
     }
-    DCHECK_EQ(_mem_tracker->consumption(), 0);

Review comment:
       Why remove this?

##########
File path: be/src/exec/partitioned_aggregation_node.cc
##########
@@ -345,9 +345,12 @@ Status PartitionedAggregationNode::open(RuntimeState* state) {
 }
 
 Status PartitionedAggregationNode::get_next(RuntimeState* state, RowBatch* row_batch, bool* eos) {
-    int first_row_idx = row_batch->num_rows();
-    RETURN_IF_ERROR(GetNextInternal(state, row_batch, eos));
-    RETURN_IF_ERROR(HandleOutputStrings(row_batch, first_row_idx));
+    DCHECK_EQ(row_batch->num_rows(), 0);
+    RowBatch batch(row_batch->row_desc(), row_batch->capacity(), _mem_tracker.get());
+    int first_row_idx = batch.num_rows();
+    RETURN_IF_ERROR(GetNextInternal(state, &batch, eos));
+    RETURN_IF_ERROR(HandleOutputStrings(&batch, first_row_idx));
+    batch.deep_copy_to(row_batch);

Review comment:
       Add comment to explain why we need deep copy here




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

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



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