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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
