Tim Armstrong has posted comments on this change. Change subject: IMPALA-3242: Remove most usages of RuntimeState::SetMemLimitExceeded() ......................................................................
Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/3140/1/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: Line 1149: int64_t alloc_size = PAGG_DEFAULT_HASH_TABLE_SZ * HashTable::BucketSize(); > The hash table hasn't been created yet before InitHashTable() is called and Right, you'd need to store int num_buckets = hash_partitions_[i]->hash_tbl->num_buckets() in a local variable before trying to Init the hash table. It's kind of annoying, I know. Anyway, this is ok just long as people won't accidentally make a change that breaks this (e.g. use non-default sizes in some cases). Line 1251: UNLIKELY > It doesn't hurt to put UNLIKELY() on uncommon error cases to streamline the I think sometimes it makes it less readable. I suppose this case isn't too bad, but more complicated conditions can get noisy. http://gerrit.cloudera.org:8080/#/c/3140/1/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: Line 862: return mem_tracker()->MemLimitExceeded(state, details, 0); > I think the aggregation code also uses the same error message so they are c Sounds good. I think I was thinking of TErrorCode::PARTITIONED_AGG_MAX_PARTITION_DEPTH, which is kind-of the same problem. -- To view, visit http://gerrit.cloudera.org:8080/3140 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic0ca128c768d1e73713866e8c513a1b75e6b4b59 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.6.0_5.8.0 Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
