impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4674: Part 2: port backend exec to BufferPool
Date Thu, 13 Jul 2017 04:43:33 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4674: Part 2: port backend exec to BufferPool

Patch Set 29:

File be/src/exec/

PS29, Line 805:     if (failed_to_add) {
              :       parent->CleanupHashTbl(agg_fn_evals, it);
              :       hash_tbl->Close();
              :       hash_tbl.reset();
              :       aggregated_row_stream->Close(NULL, RowBatch::FlushMode::NO_FLUSH_RESOURCES);
              :       RETURN_IF_ERROR(status);
              :       return Status(TErrorCode::INTERNAL_ERROR,
              :           Substitute("Unexpected error spilling aggregate node $0 to disk:
should have "
              :                      "enough memory to append to stream. $1",
              :                      parent->id_, parent->buffer_pool_client_.DebugString()));
              :     }
not ready to turn that into just a dcheck? i'm okay with leaving it for now, though not sure
how we can exercise that code.

PS29, Line 1133:     if (single_partition_idx != -1 && i != single_partition_idx)
               :       hash_partitions_.push_back(nullptr);
               :       continue;
               :     }
I think this would read easier like this:

if (single_partition_idx == -1 || i == single_partition_idx) {
   ... create partition ...
} else {

seems more obvious we just have two cases here and the positive conditional seems easier to
think about.

PS29, Line 1149: ;

PS29, Line 1152: success

PS29, Line 1254: to build from the rows of the spilled partition
not sure what that means

PS29, Line 1308: Free up a buffer's worth of
               :     // reservation
the code looks like it potentially frees up more than a buffer worth. does this mean to say
"Free up at least a buffer's worth"?

Line 1396:                    id_, buffer_pool_client_.DebugString()));
just curious: why do you expect a bug to trigger this more likely than a bug that would trigger
the DCHECK(got_memory) cases?

Line 1447:   partition->unaggregated_row_stream->UnpinStream(BufferedTupleStreamV2::UNPIN_ALL);
at this point, the streams are already unpinned right, and so we are just unpinning the iterator
pages? or no?

Line 1454:     partition->Close(true);
how about:
if (partition != nullptr) partition->Close(true);

Line 1468:     if (partition == nullptr) continue;
let's invert that since it's only one stmt in the block (even if the if-stmt doesn't fit on
one line).
File be/src/exec/partitioned-aggregation-node.h:

PS29, Line 374: success

PS29, Line 676: blocks

PS29, Line 723: one
File be/src/runtime/

PS29, Line 326: numeric_limits<int64_t>::max(
alternatively, should that use the value passed as the third param to ParseMemSpec()? I agree
this case is weird, not really sure the right thing.
File be/src/runtime/

PS26, Line 149:   bytes_limit = query_options().mem_limit;
> Filed IMPALA-5652 to deprecate it. I tested and confirmed that if you set -
hmm i'm not sure, maybe.  What about the case where there is no query mem_limit? I guess you
are saying the query buffer_pool_limit still kicks in to prevent the hogging of mem since
we use lowest_mem() above?  But isn't that also true when query mem_limit is larger than process

Actually, I'm not sure I understand the problem. Is it a new problem or pre-existing with
buffered-block-mgr too? Doesn't the buffer pool version have strictly more limits on the buffer
growth (both per query reservation limit along with the process wide buffer pool limit).
File be/src/runtime/

PS29, Line 64: starts
doesn't it start in Closed?

PS29, Line 119: ,
"and sets 'data_'  with  pointer to the page's buffer"
and then delete the second sentence since part of that is redundant and slightly confusing
("data_ is null" isn't distinct from "not already in memory" but sounds different).

PS29, Line 132: /
weird line break
File be/src/service/query-options.h:

PS29, Line 97: spillable
eventually, will these options impact non-"spillable" buffers as well? i.e. nodes that use
buffers and not pages? if so, should we remove the word spillable from them?
File fe/src/main/java/org/apache/impala/planner/

PS29, Line 120: computeResourceProfile

Line 122:   protected long spillableBufferBytes_ = -1;
maybe this should go in ResourceProfile? The reason being that the min reservation is a function
of this, so it's pretty tightly coupled, right? what do you think?

also do we have coverage where we have a lot of different buffer sizes and also large queries
causing mem pressure? basically to test the code that coalesces/breaks buffers in the real

Line 631:    * data partition.
update that comment to explain setting of the buffer size
File fe/src/main/java/org/apache/impala/planner/

PS29, Line 41: [0, Long.MAX_VALUE] are valid values.
makes me wonder what valid values for minReservationBytes_ are.  Presumably that has to be
<= maxReservationBytes_?
File tests/query_test/

Line 126:                        'Q21' : 187, 'Q22' : 125}
do you know why some of these increase? is that because of the old small buffers optimization?
File tests/query_test/

Line 38:   # Reduce the IO read size. This reduces the memory required to trigger spilling.
i guess now we rely on buffer_pool_limit for that?
File tests/stress/

PS29, Line 926: "memory limit exceeded"
I guess we have to allow that still until we bring everything else under reservations?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I7fc7fe1c04e9dfb1a0c749fb56a5e0f2bf9c6c3e
Gerrit-PatchSet: 29
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message