Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/14764 )
Change subject: IMPALA-4192: Move static state from ExecNode into a PlanNode ...................................................................... Patch Set 6: (23 comments) http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/aggregation-node-base.h File be/src/exec/aggregation-node-base.h: http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/aggregation-node-base.h@36 PS5, Line 36: /// Performs the actual work of aggregating input rows. > Comment is a little misleading. Maybe "Configuration for each aggregate cla Done http://gerrit.cloudera.org:8080/#/c/14764/3/be/src/exec/aggregation-node-base.h File be/src/exec/aggregation-node-base.h: http://gerrit.cloudera.org:8080/#/c/14764/3/be/src/exec/aggregation-node-base.h@28 PS3, Line 28: /// Base class containing common code for the ExecNodes that do aggregation, : /// AggregationNode and StreamingAggregationNode. > Shouldn't this comment be moved to AggregationNodeBase? Or reworded to fit Done. Moved all PlanNodes above the ExecNode class comment and added an overarching comment to the ExecNode. http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/aggregation-node-base.cc File be/src/exec/aggregation-node-base.cc: http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/aggregation-node-base.cc@65 PS5, Line 65: // TODO: maybe use a similar createNode method like in PlanNode. > Remove comment? The suggested refactoring seems fine, but also isn't too ne Done http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/aggregation-node-base.cc@68 PS5, Line 68: num_ags > nit: num_aggs Done http://gerrit.cloudera.org:8080/#/c/14764/3/be/src/exec/aggregation-node.cc File be/src/exec/aggregation-node.cc: http://gerrit.cloudera.org:8080/#/c/14764/3/be/src/exec/aggregation-node.cc@39 PS3, Line 39: for (int i = 0; i < pnode.tnode_->agg_node.aggregators.size(); ++i) { > Maybe a for-each loop would be more readable. Done http://gerrit.cloudera.org:8080/#/c/14764/3/be/src/exec/aggregator.h File be/src/exec/aggregator.h: http://gerrit.cloudera.org:8080/#/c/14764/3/be/src/exec/aggregator.h@81 PS3, Line 81: /// Certain aggregates require a finalize step, which is the final step of the > Maybe we could give an example of what kind of aggregator needs a finalize I am not entirely sure if this statement is correct, I briefly looked at the planner to see when this is set, seems like its only unset for the first phase of aggregation in a multi phase agg which means its not supposed to rely on the type of aggregate but rather relies on the phase of aggregation. http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h File be/src/exec/exec-node.h: http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@57 PS5, Line 57: /// TODO: Move all the static state of ExecNode into PlanNode. > Mention a JIRA here? Done http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@66 PS5, Line 66: /// do any initialization that can fail in Init() rather than the ctor. > There isn't a constructor, so comment needs fixing. I was actually wonderin will update the comment. I initially started with only moving the Exprs out of the Exec nodes and rake in other variables as needed. I eventually decided to add in the ctor once we move the rest of the static state out of the ExecNodes but it can be done in this patch too. Let me know if you feel strongly about doing it in this patch itself. http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@68 PS5, Line 68: call > nit: called Done http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@71 PS5, Line 71: pool > Do you mean state->obj_pool() or similar? Done http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@83 PS5, Line 83: const TPlanNode* tnode_ = nullptr; > It's a little weird that all of the members with underscores are public. Eventually all should be accessible by ExecNode. In that case would you recommend adding accessor methods for all or making it a friend class of ExecNode? http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@90 PS5, Line 90: RowDescriptor* row_descriptor_ = nullptr; > What is this owned by? Same for other objects. Or maybe we should document Done http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@102 PS5, Line 102: private: > Should add a DISALLOW_COPY_AND_ASSIGN(PlanNode) here to suppress copy const Done. Note: had to add an implicit default ctor for PlanNode because this macro was deleting that implicitly http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@194 PS5, Line 194: TODO: revisit this line. > ? removed http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@195 PS5, Line 195: lanNode* > should maybe be a const ref? Done http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@358 PS5, Line 358: const T& GetPlanNode() { return dynamic_cast<const T&>(plan_node_); } > DCHECK that the cast pointer isn't null? I.e. that the dynamic cast succeed this is returning a reference now so dynamic_cast on it will actually throw an exception. Also, I am eventually planning to store the specialized reference in every child class down the hierarchy to avoid doing this, since it will be accessed more often once all static state is moved to PlanNodes and accessed directly in the ExecNodes (as opposed to being copied in the ctor like it is in this patch). Thoughts? http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@382 PS5, Line 382: RowDescriptor& row_descriptor_; > const ref? Unless it results in a cascade of changes. yup, would unfortunately result in a cascade of changes http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.cc File be/src/exec/exec-node.cc: http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.cc@140 PS5, Line 140: stringstream error_msg; > Not a huge deal and you're copying the existing pattern, but I think we sho Done http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.cc@317 PS5, Line 317: plan_root > root is a little misleading since this isn't going to be the root of the wh Done http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/hdfs-scan-node-base.h File be/src/exec/hdfs-scan-node-base.h: http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/hdfs-scan-node-base.h@164 PS5, Line 164: ~HdfsScanPlanNode(){} > nit: don't really need to override the destructor Done http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/hdfs-scan-node-base.cc@109 PS5, Line 109: dynamic_cast > We should be able to use a static_cast, no? oh, didnt notice we were casting to a parent class. http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/partitioned-hash-join-node.cc@56 PS5, Line 56: // TODO: change PhjBuilder::InitExprsAndFilters to accept the runtime filter contexts > We might collide on some of this stuff, cause I'm going to be having PhjBui sounds good to me http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/runtime/fragment-instance-state.h File be/src/runtime/fragment-instance-state.h: http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/runtime/fragment-instance-state.h@153 PS5, Line 153: PlanNode* plan_tree_ = nullptr; // lives in obj_pool() > Maybe document when plan_tree_ is initialised and that it's not mutated dur Done. The comment above it would work for explaining when it is initialized. I've also made it a pointer to a const and added a comment. -- To view, visit http://gerrit.cloudera.org:8080/14764 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I69f1676bf67bac31fa5902511b3fcc269fd67472 Gerrit-Change-Number: 14764 Gerrit-PatchSet: 6 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 05 Dec 2019 01:27:40 +0000 Gerrit-HasComments: Yes
