Tim Armstrong has posted comments on this change. Change subject: IMPALA-4192: Disentangle Expr and ExprContext ......................................................................
Patch Set 14: (14 comments) Next batch - made it through everything except the expr/ subdirectory. http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: Line 164: pool_->Add(build_expr); Maybe stick with the usual idiom of wrapping pool_->Add() around the allocation? Line 1574: // TODO: consider moving the following codegen logic to AggFn. This would make sense (not in this patch though). http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/partitioned-hash-join-builder.cc File be/src/exec/partitioned-hash-join-builder.cc: Line 109: // TODO: Move to Prepare(). Is this to do in this patch or are you planning to do it as part of a future JIRA? http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/partitioned-hash-join-builder.h File be/src/exec/partitioned-hash-join-builder.h: Line 243: /// Init() function inherited from DataSink. Overridden to be a no-op for now. This is a bit of a red flag to me - I think we should structure things so that we always call Init() on the parent class. Otherwise it's hard to reason about the possible states of the member variables of DataSink. Line 381: /// List of filters to build. Mention that there's a correspondence with 'filter_exprs_'? http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: PS14, Line 95: nd and http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/runtime/data-stream-sender.h File be/src/runtime/data-stream-sender.h: Line 49: /// TODO: Move this to the equivalent of PlanNode class for DataSink. Is there a JIRA for this? http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: Line 486: DCHECK(partition_expr->IsLiteral()); Can you add a comment in CatalogObjects.thrift that these are always literal exprs? That fact is explicit in the frontend but it wasn't obvious that the exprs sent from the frontend are always literals just by looking at the thrift structures. Line 491: // literals Partition exprs are not used in the codegen case. Don't codegen Missing . http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/runtime/runtime-filter.h File be/src/runtime/runtime-filter.h: Line 93: const TRuntimeFilterDesc& filter_desc_; Comment what owns the filter? http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/runtime/sorter.h File be/src/runtime/sorter.h: Line 105: Status Init(ObjectPool* pool, RuntimeState* state, MemPool* mem_pool); Comment what the arguments are for? The names could maybe be more descriptive too. E.g. to disambiguate from 'obj_pool_'. E.g. 'expr_evaluator_pool' and 'expr_mem_pool'. I think passing in 'state' is redundant too, since it's passed into the constructor. http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/service/fe-support.cc File be/src/service/fe-support.cc: PS14, Line 194: mem_pool expr_mem_pool? http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/udf/udf.h File be/src/udf/udf.h: Line 111: /// TODO: Move FRAGMENT_LOCAL states to query_state for multi-threading. Do we have a JIRA trackign this? http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/util/tuple-row-compare.h File be/src/util/tuple-row-compare.h: Line 82: Status Open(ObjectPool* pool, RuntimeState* state, MemPool* mem_pool); Similar to my comment in Sorter - it's unclear what these different arguments do and what the requirements are. E.g. what is allocated from 'mem_pool'. -- To view, visit http://gerrit.cloudera.org:8080/5483 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iefdc9aeeba033355cb9497e3a5d2363627dcf2f3 Gerrit-PatchSet: 14 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
