Tim Armstrong 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 9: Code-Review+1 (6 comments) A couple of very minor comments. I can upgrade to a +2 once other reviewers have had a chance to finish their reviews. 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@66 PS5, Line 66: public: > will update the comment. I initially started with only moving the Exprs out I'm fine if you stage the work in the way you think will be most efficient, I don't want to mess with your process, just understand the next steps. http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@83 PS5, Line 83: > Eventually all should be accessible by ExecNode. In that case would you rec Maybe make it a friend class of ExecNode? And leave a TODO to clean that up in a later patch. http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@358 PS5, Line 358: return reservation_manager_.ReleaseUnusedReservation(); > this is returning a reference now so dynamic_cast on it will actually throw Oh I didn't know that subtlety about dynamic_cast. This seems fine then as an intermediate step. http://gerrit.cloudera.org:8080/#/c/14764/5/be/src/exec/exec-node.h@382 PS5, Line 382: > yup, would unfortunately result in a cascade of changes ok, no prob. Leave a TODO maybe just so it's clear that it's not intentionally mutable. http://gerrit.cloudera.org:8080/#/c/14764/9/be/src/exec/hdfs-scan-node-base.cc File be/src/exec/hdfs-scan-node-base.cc: http://gerrit.cloudera.org:8080/#/c/14764/9/be/src/exec/hdfs-scan-node-base.cc@89 PS9, Line 89: const TTupleId& tuple_id =tnode.hdfs_scan_node.tuple_id; nit: missing space http://gerrit.cloudera.org:8080/#/c/14764/9/be/src/exec/scan-node.cc File be/src/exec/scan-node.cc: http://gerrit.cloudera.org:8080/#/c/14764/9/be/src/exec/scan-node.cc@124 PS9, Line 124: // TODO: add the name of the node type This doesn't seem that necessary, the value should be enough to debug - maybe remove the TODO so there's less noise. -- 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: 9 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 21:32:19 +0000 Gerrit-HasComments: Yes
