Csaba Ringhofer 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 (5 comments) Looks good to me, but I didn't read through all changes carefully. I have some suggestions to improve readability, they are optional, but I think this is a good point to do such refactors. 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 codegened function pointers may become mutable eventually (starting as null and later replaced with actual pointers). It would be good to know what kind of members are planned to go into PlanNode in the future, e.g.: a. static members that do not change after Init() b. static members that can change but won't change often (e.g. runtime filters, codegened functions in the future) c. static members that change often (are there any?) a. seems obvious, b. questionable, while c. could be potentially harmful for performance due to false sharing 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 http://gerrit.cloudera.org:8080/#/c/14764/9/be/src/exec/analytic-eval-node.cc@103 PS9, Line 103: analytic_node here and at other places with the same pattern: I think it would be much more readable if we would pass pnode.tnode_->analytic_node as an argument. If this was a normal function, then getting it from the argument at the beginning would be probably better, but as we want to use it in the initializer list, the extra argument seems the best way to reduce clutter. http://gerrit.cloudera.org:8080/#/c/14764/9/be/src/exec/analytic-eval-node.cc@147 PS9, Line 147: const AnalyticEvalPlanNode& analytic_node = GetPlanNode<AnalyticEvalPlanNode>(); here and at other places with the same pattern: would there be any drawback in passing the exact class (here AnalyticEvalPlanNode) for the constructor instead of PlanNode? The CreateNode functions know the exact class + I see no benefit in the unified signature of the constructor. 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@364 PS9, Line 364: move typo: moved -- 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: Tue, 10 Dec 2019 16:01:33 +0000 Gerrit-HasComments: Yes
