[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs

["Csaba Ringhofer (Code Review)" <ge...@cloudera.org>]

Csaba Ringhofer 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 benchmarks for that too.


http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG@43
PS6, Line 43: The CHAR-specific
nit: wrap at 72 chars


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?


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.


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 subclasses.


http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-expr.h@265
PS6, Line 265: Interpretd
typo


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.


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


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


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 both in declaration and implementation, and have a single macro that calls it for all *Val that supports codegen. This would make it easier to avoid mistakes when adding a new type, e.g. Date.



-- 
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 <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 05 Apr 2019 16:30:33 +0000
Gerrit-HasComments: Yes