impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-2020, IMPALA-4809: Codegen support for DECIMAL V2
Date Fri, 10 Feb 2017 08:42:03 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message