Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs ...................................................................... Patch Set 6: (10 comments) http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG@41 PS6, Line 41: IMPALA-7331 (CHAR codegen support functions) is also fixed becaus > Shouldn't this speed up CHAR(N) manipulation? It would be nice to see some I added a toy manual benchmark below that shows a significant improvement. Do you think it's worth adding something to targeted-perf? It feels a bit contrived unless we have a perf dataset with CHAR columns. http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG@43 PS6, Line 43: The CHAR-specific > nit: wrap at 72 chars Done http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG@53 PS6, Line 53: * Ran a basic insert benchmark, which went from 10.1s to 7.6s : create table foo stored as parquet as : select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end : from tpch30_parquet.lineitem; : * Added perf regression test to tpcds-insert, similar to the manual : benchmark. > Is there a specific reason for using INSERT for benchmarking? INSERT was one of the main motivators for this work, e.g. see IMPALA-4358. http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/codegen/codegen-anyval.h File be/src/codegen/codegen-anyval.h: http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/codegen/codegen-anyval.h@52 PS6, Line 52: /// TYPE_STRING,TYPE_VARCHAR,TYPE_CHAR,TYPE_FIXED_UDA_INTERMEDIATE/StringVal: { i64, i8* } > Collections could be also mentioned here. Done http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-expr.h File be/src/exprs/scalar-expr.h: http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-expr.h@85 PS6, Line 85: One is implemented for each : /// possible return type it supports (e.g. GetBooleanVal(), GetStringVal(), etc). The : /// return type is a subclass of AnyVal (e.g. StringVal). One or more of these compute : /// functions must be overridden by subclasses of ScalarExpr. > In the new version the *Interpreted() functions have to be overriden by sub Done http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-expr.h@265 PS6, Line 265: Interpretd > typo Done http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.h File be/src/exprs/scalar-fn-call.h: http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.h@47 PS6, Line 47: GetCodegendComputeFn > "Impl" is missing. Done http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.h@48 PS6, Line 48: e > nit: long line could be fixed if this comment is touched Done http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.h@49 PS6, Line 49: If codegen is enabled, ScalarFnCall's Get*Val() compute : /// functions are wrappers around this codegen'd function. > This sentence would be more relevant in scalar-expr.h I think the updated comment in scalar-expr.h covers this, so just deleted this sentence. http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc@483 PS6, Line 483: // TODO: macroify this? > +1 for this TODO, I think that it would be better to do this with macros bo Done -- To view, visit http://gerrit.cloudera.org:8080/12797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11 Gerrit-Change-Number: 12797 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Sat, 06 Apr 2019 00:41:38 +0000 Gerrit-HasComments: Yes
