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 10: (9 comments) http://gerrit.cloudera.org:8080/#/c/14764/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14764/9//COMMIT_MSG@18 PS9, Line 18: Eventually all static state including codegened function pointers : would be moved to the PlanNodes. > We have plans with Daniel Becker to change codegen to be async, so the code like you said the answer is (a), I did however consider async codegen in my design and the way I had planned to implement it is by using an atomic pointer. Currently during execution we already check for every pass if the function pointer is null or not, with this change it would have a reference to an atomic pointer which would we updated by codegen as soon as codegen is complete and even if execution missed a few passes it would be harmless. http://gerrit.cloudera.org:8080/#/c/14764/9/be/src/exec/analytic-eval-node.cc File be/src/exec/analytic-eval-node.cc: http://gerrit.cloudera.org:8080/#/c/14764/9/be/src/exec/analytic-eval-node.cc@50 PS9, Line 50: state->desc_tbl().GetTupleDescriptor(tnode.analytic_node.intermediate_tuple_id); : TupleDescriptor* result_tuple_desc = : state->desc_tbl().GetTupleDescriptor(tnode.analytic_node.output_tuple_id); > nit: -2 indentation Done http://gerrit.cloudera.org:8080/#/c/14764/9/be/src/exec/analytic-eval-node.cc@103 PS9, Line 103: descs), > here and at other places with the same pattern: I think it would be much mo Done http://gerrit.cloudera.org:8080/#/c/14764/9/be/src/exec/analytic-eval-node.cc@147 PS9, Line 147: // Set the Exprs from the PlanNode > here and at other places with the same pattern: would there be any drawback excellent suggestion. Done. http://gerrit.cloudera.org:8080/#/c/14764/9/be/src/exec/exec-node.h File be/src/exec/exec-node.h: http://gerrit.cloudera.org:8080/#/c/14764/9/be/src/exec/exec-node.h@76 PS9, Line 76: virtual Status CreateExecNode(RuntimeState* state, ExecNode** node) const = 0; > Just an annoying last minute nit, feel free to ignore if you don't agree: s Done http://gerrit.cloudera.org:8080/#/c/14764/9/be/src/exec/exec-node.h@364 PS9, Line 364: re a > typo: moved Done 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@83 PS5, Line 83: > Maybe make it a friend class of ExecNode? And leave a TODO to clean that up Just added a TODO for now since friendship and inheritance dont go that well and will make the code really unwieldy. 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 Done 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: DCHECK(false) << "Unexpected scan node type: " << tnode_->node_type; > This doesn't seem that necessary, the value should be enough to debug - may Done -- 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: 10 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, 12 Dec 2019 02:33:35 +0000 Gerrit-HasComments: Yes
