impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4809: Codegen support for DECIMAL V2
Date Fri, 10 Feb 2017 00:59:46 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4809: Codegen support for DECIMAL_V2
......................................................................


Patch Set 1:

(16 comments)

Did you verify performance of the decimal->decimal CAST path to make sure constant injection
got rid of all those branches?

http://gerrit.cloudera.org:8080/#/c/5950/1//COMMIT_MSG
Commit Message:

Line 21: 
also mention that this does part of IMPALA-2020 for decimal->decimal casts.


http://gerrit.cloudera.org:8080/#/c/5950/1/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

Line 468:       boost::unordered_set<std::string>* symbols);
why this change? where's it used?


http://gerrit.cloudera.org:8080/#/c/5950/1/be/src/exec/partitioned-aggregation-node.cc
File be/src/exec/partitioned-aggregation-node.cc:

PS1, Line 1740: output_type
Tim fixed this as well in https://gerrit.cloudera.org/#/c/5161/23. Same fix but factored differently.
I think he added a test as well.  Anyway, you're going to conflict here so maybe you should
rebase on his change (which we need anyway)?


http://gerrit.cloudera.org:8080/#/c/5950/1/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:

PS1, Line 478: output_precision < val_precision
this gets codegen'ed away, right?


Line 479:       abs(result.val16) >=  DecimalUtil::GetScaleMultiplier<int128_t>(output_precision))
{
how about using RETURN_IF_OVERFLOW() so we get the warning (and later it makes it easier in
case we need to change overflow handling for strict mode).


PS1, Line 505: static_cast<bool>(is_decimal_v2)
usually we convert to bool like this: decimal_v2 != 0


http://gerrit.cloudera.org:8080/#/c/5950/1/be/src/exprs/expr-codegen-test.cc
File be/src/exprs/expr-codegen-test.cc:

Line 310:   int64_t v = 1000025;
what's this?


PS1, Line 316: 100003
and this?


http://gerrit.cloudera.org:8080/#/c/5950/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 1334:     {{ false, -999999999, 9, 0 }, { true, 0, 9, 0 }} },
how about a positive/negative test where the digit is 5 (to show round-up)?


PS1, Line 1357: v2
sorry about that


http://gerrit.cloudera.org:8080/#/c/5950/1/be/src/udf/udf-internal.h
File be/src/udf/udf-internal.h:

PS1, Line 131: attributes of the return type and argument types of a UDF/UDA
static attributes of the UDF/UDA that can be injected as constants by codegen.


PS1, Line 148:  This function returns the various attributes of the return type and argument
types
             :   /// recorded in 'ctx'
             :   /// It also returns the value of the query option decimal_v2 which dictates
the behavior
             :   /// of cast to Decimal type
How about just say

This function returns the various static attributes of the UDF/UDA.  Calls to this function
are replaced by constants injected by codegen.

that way, we don't have to update it every time we add an enum, which can document the specific
attributes.


PS1, Line 160:  /// Functions which implement the UDF or UDA interface should use this function
so that
             :   /// runtime constants of the type attributes would be inlined when codegen
is enabled.
this can be deleted if you use the wording above (and it's good to mention codegen upfront
because otherwise it's not clear why we'd have this indirection).


Line 170:   /// - whether query option decimal_v2 is set (returns 0 or 1)
could just refer to ConstFnAttr.


http://gerrit.cloudera.org:8080/#/c/5950/1/testdata/workloads/functional-query/queries/QueryTest/decimal.test
File testdata/workloads/functional-query/queries/QueryTest/decimal.test:

Line 469: ---- RESULTS
how about running this query with v2=false as well so the difference is clear?


Line 495: ====
this is going to conflict with my change, which causes us to run decimal.test for both v2={0,1}.
 I introduced decimal-exprs.test for cases we want to test distinct behavior of each expression,
so how about moving these there?


-- 
To view, visit http://gerrit.cloudera.org:8080/5950
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2434d240f65b81389b8a8ba027f980a0e1d1f981
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message