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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-HasComments: Yes
