Wenzhe Zhou has posted comments on this change. ( http://gerrit.cloudera.org:8080/13645 )
Change subject: WIP IMPALA-6590: Disable expr rewrites and codegen for VALUES() statements ...................................................................... Patch Set 5: (7 comments) 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 Will add new frontend tests 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: bool is_codegen_status_added_ = false; > Can you mention that this is only used for observability? Or something alon Done 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: /// it the ScalarExprEvaluator and TupleRow. These are cross-compiled and used by > Could be const since it always initialised in the constructor. Done 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: > Please use an int64_t - the style guide says to stick to signed integers (w Done 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: 3: required i32 num_children > I was thinking about whether it would be better to invert the meaning of th ok http://gerrit.cloudera.org:8080/#/c/13645/1/common/thrift/Exprs.thrift@154 PS1, Line 154: > I would avoid renumbering the fields. you could just make this 22. Tried to move the new field to the last spot, but did not work. Backend c++ code always get the value as false even the frontend set it as true. 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: private boolean isConstant_; > Non-standard formatting. If we use braces, we generally just put the statem done -- 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: 5 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 30 Nov 2021 21:50:10 +0000 Gerrit-HasComments: Yes
