Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13645 )
Change subject: IMPALA-6590: Disable expr rewrites and codegen for VALUES() statements ...................................................................... Patch Set 1: (7 comments) The approach makes sense. http://gerrit.cloudera.org:8080/#/c/13645/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13645/1//COMMIT_MSG@38 PS1, Line 38: Results below. I think we should have some frontend tests for the expression rewrite part of this. I don't think anything too elaborate, but just queries with values clauses in a couple of positions (e.g. a simple INSERT, and maybe a VALUE clause as a subquery in a select), with expressions that would have otherwise been rewritten. I think that would fit in ./fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java http://gerrit.cloudera.org:8080/#/c/13645/1/be/src/exec/union-node.h File be/src/exec/union-node.h: http://gerrit.cloudera.org:8080/#/c/13645/1/be/src/exec/union-node.h@70 PS1, Line 70: /// Number of const scalar expressions which will be codegened. Can you mention that this is only used for observability? Or something along those lines. It gets confusing otherwise with all the state variables - it's hard to understand what actually influences the control flow, etc. http://gerrit.cloudera.org:8080/#/c/13645/1/be/src/exprs/scalar-expr.h File be/src/exprs/scalar-expr.h: http://gerrit.cloudera.org:8080/#/c/13645/1/be/src/exprs/scalar-expr.h@402 PS1, Line 402: bool is_codegen_disabled_; Could be const since it always initialised in the constructor. http://gerrit.cloudera.org:8080/#/c/13645/1/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: http://gerrit.cloudera.org:8080/#/c/13645/1/be/src/runtime/runtime-state.h@155 PS1, Line 155: uint32_t Please use an int64_t - the style guide says to stick to signed integers (with a few exceptions) https://google.github.io/styleguide/cppguide.html#Integer_Types http://gerrit.cloudera.org:8080/#/c/13645/1/common/thrift/Exprs.thrift File common/thrift/Exprs.thrift: http://gerrit.cloudera.org:8080/#/c/13645/1/common/thrift/Exprs.thrift@153 PS1, Line 153: // If codegen is disabled for this Expr I was thinking about whether it would be better to invert the meaning of this field, i.e. make it is_codegen_enabled, since it's often easier to follow the code - you don't have to parse double negatives like !is_codegen_disabled. I think this is OK, particularly since "codegen disabled" is used pretty pervasively in Impala. http://gerrit.cloudera.org:8080/#/c/13645/1/common/thrift/Exprs.thrift@154 PS1, Line 154: 5: required bool is_codegen_disabled I would avoid renumbering the fields. you could just make this 22. It doesn't *really* matter for this struct since it's only used internally between daemons running the same version, but renumbering thrift fields does break binary compatibility, with weird errors. So it's a bad habit to get into, I think. http://gerrit.cloudera.org:8080/#/c/13645/1/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/13645/1/fe/src/main/java/org/apache/impala/analysis/Expr.java@400 PS1, Line 400: for (Expr child: children_) {child.disableCodegen();} Non-standard formatting. If we use braces, we generally just put the statement on multiple lines. -- To view, visit http://gerrit.cloudera.org:8080/13645 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I229d67b821968321abd8f97f7c89cf2617000d8d Gerrit-Change-Number: 13645 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 14 Jun 2019 16:05:45 +0000 Gerrit-HasComments: Yes
