Michael Ho has posted comments on this change. Change subject: IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL_V2 ......................................................................
Patch Set 1: (16 comments) 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. Done 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); > never mind, i think i saw this. it's only used in one place and it just mov Yes. This change allows CreateFrom* to be all private functions of LlvmCodeGen. 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 > or, after seeing the full patch, i think you can just revert this to avoid Reverted and merged with Tim's patch. 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? Yes, it will. 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 ma Done PS1, Line 505: static_cast<bool>(is_decimal_v2) > usually we convert to bool like this: decimal_v2 != 0 Done 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? Oh, it's just verifying the rounding actually occurred with decimal_v2. PS1, Line 316: 100003 > and this? Comments added. 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)? Modified the test above to show rounding of 0.12344 vs 0.12345. Is that what you mean ? PS1, Line 1357: v2 > sorry about that No problem. I missed that in the review too. 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 codeg Done 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 Done 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 Done Line 170: /// - whether query option decimal_v2 is set (returns 0 or 1) > could just refer to ConstFnAttr. Done 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 cle The query above was run with v2=false. Did you mean something else ? Line 495: ==== > this is going to conflict with my change, which causes us to run decimal.te Yes, we should coordinate. If your change merges first, I will move it over to the new file which doesn't exist yet in trunk. -- 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 <k...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-HasComments: Yes
