Joe McDonnell 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 10: Code-Review+1 (3 comments) This makes sense to me. I only have a couple nits, but otherwise I'm good to go to +2. http://gerrit.cloudera.org:8080/#/c/13645/10/be/src/exec/union-node.cc File be/src/exec/union-node.cc: http://gerrit.cloudera.org:8080/#/c/13645/10/be/src/exec/union-node.cc@44 PS10, Line 44: const int64_t num_scalar_expr_to_be_codegened = state->NumScalarExprNeedsCodegen(); Nit: Should we call this "num_nonconst_scalar_expr_to_be_codegened"? It might make the calculation below clearer. Alternatively, we could just put a comment in saying that NumScalarExprNeedsCodegen() changes as we add the const exprs. http://gerrit.cloudera.org:8080/#/c/13645/10/be/src/exprs/scalar-expr.h File be/src/exprs/scalar-expr.h: http://gerrit.cloudera.org:8080/#/c/13645/10/be/src/exprs/scalar-expr.h@259 PS10, Line 259: bool is_codegen_disabled Nit: One option is to give is_codegen_disabled a default value of false. This would let you avoid some of the changes in other files. ScalarExpr(const ColumnType& type, bool is_constant, bool is_codegen_disabled = false); I don't have a strong preference about this. http://gerrit.cloudera.org:8080/#/c/13645/10/tests/query_test/test_codegen.py File tests/query_test/test_codegen.py: http://gerrit.cloudera.org:8080/#/c/13645/10/tests/query_test/test_codegen.py@96 PS10, Line 96: def test_const_scalar_expr_in_union(self, vector): Nit: Since this creates tables, it would be good to use a unique_database so the tables automatically get cleaned up. -- 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: 10 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Wed, 15 Dec 2021 20:23:00 +0000 Gerrit-HasComments: Yes
