impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zach Amsden (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-2020: Inline big number strings
Date Thu, 02 Mar 2017 21:03:48 GMT
Zach Amsden has posted comments on this change.

Change subject: IMPALA-2020: Inline big number strings

Patch Set 5:

Commit Message:

PS5, Line 27: Query: select sum(cast(cast(l_extendedprice as double) as
            : decimal(14,2))) from tpch10_parquet.lineitem
            : Query submitted at: 2017-02-18 01:23:54 (Coordinator:
            : http://impala-dev:25000)
            : Query progress can be monitored at:
            : http://impala-dev:25000/query_plan?query_id=f0410a35e981a86b:e6d9207000000000
> elide

PS5, Line 38: 0.83s
> error bars?
what do you mean exactly?
File be/src/exprs/expr-value.h:

Line 177:             decimal4_val = +DecimalUtil::MAX_UNSCALED_DECIMAL4;
> Why do you want to convert it to an Rvalue? Just to avoid having to define 
I just defined it in the .cc - not doing so (and not converting to an rvalue) breaks the debug
build, but not the release build, as this does actually get inlined if defined in the header.
File be/src/util/decimal-util.h:

Line 57:   template<typename T>
> Describe what it does.

PS5, Line 58: T
> Always is_integral?
Not necessarily, this would work for float / double as well.

PS5, Line 59: boost::
> static_assert is now part of the language, but I'm not sure if it works on 
I'll try it

Line 60:     return scale == 0 ? 1 : 10 * GetConstScaleMultiplier<T>(scale-1);
> Check for no overflow, if it can be done simply.
I think it can be done, not sure how simply.  Since this is constexpr, we are pretty constrained
in what we can use but I think another static assert could do it.

Line 136:   if (__builtin_const_p(scale) && p < 10) return GetConstScaleMultiplier<int32_t>(scale);
> Does this by itself speed anything up? If scale is constant at compile time
Let's remove it and find out.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I5095a366d914cebb0b64bd434a08dbb55c90ed30
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Zach Amsden <>
Gerrit-HasComments: Yes

View raw message