You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2016/09/07 16:36:13 UTC

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Tim Armstrong has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4326

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................

IMPALA-4008: don't bake in hash table and hash join pointers

This fixes some of the cases where memory addresses are baked into
codegen'd code.

Testing:
Ran exhaustive build.

Perf:
Ran a local perf run. No significant changes. I was able to see some
small improvements on microbenchmarks.

    +-----------+-----------------------+---------+------------+------------+----------------+
    | Workload  | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
    +-----------+-----------------------+---------+------------+------------+----------------+
    | TPCH(_20) | parquet / none / none | 9.07    | +0.46%     | 5.88       | +0.34%         |
    +-----------+-----------------------+---------+------------+------------+----------------+

    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | Workload  | Query    | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | TPCH(_20) | TPCH-Q2  | parquet / none / none | 2.12   | 1.89        |   +12.29%  | * 10.85% * | * 20.30% *     | 1           | 10    |
    | TPCH(_20) | TPCH-Q13 | parquet / none / none | 9.84   | 9.34        |   +5.39%   |   9.01%    |   3.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q17 | parquet / none / none | 14.61  | 14.19       |   +2.97%   |   2.15%    |   1.52%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q18 | parquet / none / none | 14.76  | 14.35       |   +2.82%   |   3.20%    |   2.64%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q9  | parquet / none / none | 13.72  | 13.54       |   +1.30%   |   1.75%    |   0.70%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q8  | parquet / none / none | 5.71   | 5.64        |   +1.30%   |   1.21%    |   1.23%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q19 | parquet / none / none | 47.35  | 46.75       |   +1.28%   |   2.39%    |   1.88%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q5  | parquet / none / none | 4.57   | 4.52        |   +1.20%   |   1.30%    |   0.88%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q16 | parquet / none / none | 2.07   | 2.05        |   +1.12%   |   2.59%    |   1.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q11 | parquet / none / none | 1.45   | 1.45        |   +0.15%   |   2.69%    |   2.06%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q3  | parquet / none / none | 4.65   | 4.65        |   -0.09%   |   2.12%    |   2.17%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q4  | parquet / none / none | 3.22   | 3.23        |   -0.26%   |   1.03%    |   1.33%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q7  | parquet / none / none | 15.84  | 15.92       |   -0.50%   |   0.91%    |   1.15%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q14 | parquet / none / none | 3.29   | 3.31        |   -0.59%   |   3.31%    |   1.58%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q22 | parquet / none / none | 2.65   | 2.67        |   -0.78%   |   3.03%    |   1.46%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q15 | parquet / none / none | 4.50   | 4.55        |   -1.19%   |   2.87%    |   2.45%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q20 | parquet / none / none | 3.84   | 3.91        |   -1.76%   |   2.20%    |   1.94%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q10 | parquet / none / none | 5.58   | 5.70        |   -2.00%   |   1.01%    |   1.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q21 | parquet / none / none | 22.84  | 23.42       |   -2.47%   |   0.68%    |   0.56%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q1  | parquet / none / none | 11.25  | 11.60       |   -3.06%   |   0.48%    |   1.74%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q12 | parquet / none / none | 3.81   | 3.98        |   -4.38%   |   1.62%    |   1.14%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q6  | parquet / none / none | 1.94   | 2.04        |   -4.85%   |   2.40%    |   1.58%        | 1           | 10    |
    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+

    +--------------------+-----------------------+---------+------------+------------+----------------+
    | Workload           | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
    +--------------------+-----------------------+---------+------------+------------+----------------+
    | TARGETED-PERF(_20) | parquet / none / none | 8.17    | -1.66%     | 2.96       | -1.48%         |
    +--------------------+-----------------------+---------+------------+------------+----------------+

    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | Workload           | Query                                                  | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | TARGETED-PERF(_20) | primitive_topn_bigint                                  | parquet / none / none | 3.32   | 2.87        |   +15.31%  | * 17.22% * |   1.64%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q4                                            | parquet / none / none | 7.07   | 6.61        |   +6.93%   | * 15.59% * |   5.08%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q1                                            | parquet / none / none | 1.18   | 1.12        |   +5.57%   |   1.94%    |   2.96%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_empty_build_join_1                           | parquet / none / none | 10.75  | 10.47       |   +2.76%   |   1.15%    |   0.94%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_top-n_all                                    | parquet / none / none | 24.30  | 23.85       |   +1.87%   |   1.40%    |   0.82%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_2                             | parquet / none / none | 2.45   | 2.42        |   +1.38%   |   1.93%    |   1.33%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_bigint_selective                      | parquet / none / none | 0.57   | 0.57        |   +1.16%   |   3.66%    |   4.19%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q3                                         | parquet / none / none | 1.70   | 1.68        |   +1.06%   |   1.88%    |   2.69%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_3                             | parquet / none / none | 4.15   | 4.13        |   +0.47%   |   1.27%    |   1.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_1                             | parquet / none / none | 1.46   | 1.46        |   +0.32%   |   1.68%    |   2.43%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_orderby_all                                  | parquet / none / none | 12.92  | 12.89       |   +0.22%   |   1.69%    |   1.12%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q4                                         | parquet / none / none | 1.68   | 1.67        |   +0.21%   |   2.31%    |   2.44%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q7                                         | parquet / none / none | 3.35   | 3.35        |   +0.14%   |   1.10%    |   1.84%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_4                          | parquet / none / none | 0.46   | 0.46        |   +0.07%   |   0.29%    |   0.27%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_pk                            | parquet / none / none | 35.99  | 36.04       |   -0.13%   |   8.55%    |   6.95%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q6                                            | parquet / none / none | 0.97   | 0.97        |   -0.16%   |   2.68%    |   2.66%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_decimal_selective                     | parquet / none / none | 0.84   | 0.84        |   -0.31%   |   2.85%    |   3.58%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_shuffle_join_union_all_with_groupby          | parquet / none / none | 20.46  | 20.53       |   -0.34%   |   0.60%    |   0.60%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q7                                            | parquet / none / none | 0.98   | 0.98        |   -0.44%   |   2.78%    |   2.33%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_exchange_shuffle                             | parquet / none / none | 25.21  | 25.34       |   -0.49%   |   1.94%    |   1.30%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_like                           | parquet / none / none | 6.27   | 6.31        |   -0.56%   |   0.59%    |   0.43%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q5                                         | parquet / none / none | 1.96   | 1.97        |   -0.60%   |   1.98%    |   1.25%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_bigint_non_selective                  | parquet / none / none | 0.53   | 0.53        |   -0.71%   |   2.64%    |   0.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_exchange_broadcast                           | parquet / none / none | 14.77  | 14.97       |   -1.33%   |   2.59%    |   2.39%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_LIMIT-Q1                                          | parquet / none / none | 0.01   | 0.01        |   -1.40%   |   3.77%    |   3.51%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_orderby_bigint                               | parquet / none / none | 2.58   | 2.61        |   -1.44%   |   2.85%    |   1.04%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q1                                         | parquet / none / none | 1.47   | 1.49        |   -1.63%   |   2.79%    |   1.62%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_decimal_non_selective                 | parquet / none / none | 0.79   | 0.81        |   -1.65%   |   1.91%    |   2.67%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q2                                         | parquet / none / none | 1.56   | 1.58        |   -1.73%   |   2.86%    |   2.57%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q6                                         | parquet / none / none | 4.48   | 4.59        |   -2.22%   |   1.21%    |   0.85%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_5                          | parquet / none / none | 11.80  | 12.10       |   -2.50%   |   3.06%    |   2.09%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_1                          | parquet / none / none | 7.96   | 8.18        |   -2.58%   |   1.69%    |   2.17%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_shuffle_join_one_to_many_string_with_groupby | parquet / none / none | 93.11  | 95.78       |   -2.78%   |   1.07%    |   1.00%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_selective                      | parquet / none / none | 0.88   | 0.91        |   -3.03%   |   4.68%    |   5.34%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q5                                            | parquet / none / none | 2.44   | 2.52        |   -3.35%   |   2.39%    |   2.04%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_highndv                       | parquet / none / none | 9.16   | 9.50        |   -3.59%   |   0.98%    |   1.56%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_3                          | parquet / none / none | 1.44   | 1.50        |   -4.14%   |   1.10%    |   1.08%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_non_selective                  | parquet / none / none | 0.89   | 0.93        |   -4.31%   |   7.20%    |   5.12%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_decimal_highndv                      | parquet / none / none | 13.98  | 14.94       |   -6.40%   | * 11.24% * |   8.66%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q2                                            | parquet / none / none | 2.87   | 3.13        |   -8.36%   |   0.57%    |   1.59%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_lowndv                        | parquet / none / none | 1.48   | 1.62        |   -8.91%   |   1.62%    |   1.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_2                          | parquet / none / none | 13.68  | 15.05       |   -9.14%   |   2.91%    |   1.83%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_decimal_lowndv.test                  | parquet / none / none | 1.47   | 1.61        |   -9.26%   |   0.19%    |   1.50%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q3                                            | parquet / none / none | 4.25   | 4.82        |   -11.82%  |   0.79%    |   0.84%        | 1           | 10    |
    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+

Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
---
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
M be/src/exec/hash-table.inline.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node.cc
5 files changed, 224 insertions(+), 239 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/4326/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4326
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4326/4/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:

Line 308:     uint8_t* ALWAYS_INLINE cur_expr_values() const { return cur_expr_values_; }
> let's add a short comment here too:
Done


PS4, Line 310: .
> for the current row.
Done


PS4, Line 400: into
> This one should have stayed "in" (or "of"), right? i.e. it doesn't write *e
Done


Line 427:   uint32_t HashVariableLenRow(uint8_t* expr_values, uint8_t* expr_values_null) const;
> how about making these const as well.
Done


Line 441:       TupleRow* build_row, uint8_t* expr_values, uint8_t* expr_values_null) const;
> these too could be const to clarify they are "in" params.
Done. Also made the various TupleRow arguments const too for consistency. This all led to a few additional consts.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 2:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/4326/2/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

Line 185: uint32_t HashTableCtx::HashVariableLenRow(
> nit: this new line seems to make the formatting a bit inconsistent with say
The whitespace decisions were made by clang-format. It does seem a little different to what we did elsewhere but seems readable enough - not sure its worth fighting it.


PS2, Line 317: uint8_t*
> const uint8_t*
Some of the callsites need it to return a non-const pointer, e.g. EvalRow().


PS2, Line 322: ExprValueNullPtr(
> This function seems to have only one caller (ExprValueNull()) left. I find 
Done


Line 967:     return Status(
> Wondering why the extra new line here ?
clang-format gets a bit funny with string literals. I joined the string literal below so that it fits on 2 lines (the minimum).


http://gerrit.cloudera.org:8080/#/c/4326/2/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:

Line 158:     return expr_values_cache_.ExprValuePtr(expr_values_cache_.cur_expr_values(), expr_idx);
> nit: long line
Done


PS2, Line 406: i
> missing space ?
Done


PS2, Line 407:  
> extra space
Done


Line 419:   bool IR_NO_INLINE EvalProbeRow(
> nit: this extra new line seems a bit inconsistent with the rest of the file
Done


PS2, Line 425: rows
> a row ?
Done


Line 429:   /// 'expr_values_null'
> Extra new line again. Why not continue the next line here ?
Done


PS2, Line 436:  
> nit: extra space
Done


PS2, Line 446: IR_ALWAYS_INLINE 
> ALWAYS_INLINE ?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 3:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/4326/3/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:

PS3, Line 300: ExprValuesHash
CurExprValuesHash()  [see below for motiviation]


PS3, Line 303: SetExprValuesHash
Let's rename this to more closely match cur_expr_values() rather than matching ExprValuePtr (which used to make sense, but now no longer does given that ExprValuePtr doesn't necessarily operate on "current").

SetCurExprValuesHash()?


PS3, Line 309: cur_expr_values_null
comment explaining that there is one byte per null value (i.e. these are used as bools, but typed as uint8_t for codegen).  (to support the arithmetic above).


PS3, Line 403: in
the first time I read this, I read it as this functions stores the values that are in 'expr_values'.  Let's clarify by saying "to" or "into".


PS3, Line 403: in
same


PS3, Line 413: in
same


PS3, Line 431: This will be replaced by
             :   /// codegen.
nit: move this sentence to end of the comment so it's standardized with similar comments and not buried.


PS3, Line 438: with 'cur_expr_values_'
"to compare against the current row."  (since it also uses cur_expr_values_null and to give a little more info than what the C code tells us).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#3).

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................

IMPALA-4008: don't bake in hash table and hash join pointers

This fixes some of the cases where memory addresses are baked into
codegen'd code.

Testing:
Ran exhaustive build.

Perf:
Ran a local perf run. No significant changes. I was able to see some
small improvements on microbenchmarks.

    +-----------+-----------------------+---------+------------+------------+----------------+
    | Workload  | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
    +-----------+-----------------------+---------+------------+------------+----------------+
    | TPCH(_20) | parquet / none / none | 9.07    | +0.46%     | 5.88       | +0.34%         |
    +-----------+-----------------------+---------+------------+------------+----------------+

    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | Workload  | Query    | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | TPCH(_20) | TPCH-Q2  | parquet / none / none | 2.12   | 1.89        |   +12.29%  | * 10.85% * | * 20.30% *     | 1           | 10    |
    | TPCH(_20) | TPCH-Q13 | parquet / none / none | 9.84   | 9.34        |   +5.39%   |   9.01%    |   3.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q17 | parquet / none / none | 14.61  | 14.19       |   +2.97%   |   2.15%    |   1.52%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q18 | parquet / none / none | 14.76  | 14.35       |   +2.82%   |   3.20%    |   2.64%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q9  | parquet / none / none | 13.72  | 13.54       |   +1.30%   |   1.75%    |   0.70%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q8  | parquet / none / none | 5.71   | 5.64        |   +1.30%   |   1.21%    |   1.23%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q19 | parquet / none / none | 47.35  | 46.75       |   +1.28%   |   2.39%    |   1.88%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q5  | parquet / none / none | 4.57   | 4.52        |   +1.20%   |   1.30%    |   0.88%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q16 | parquet / none / none | 2.07   | 2.05        |   +1.12%   |   2.59%    |   1.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q11 | parquet / none / none | 1.45   | 1.45        |   +0.15%   |   2.69%    |   2.06%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q3  | parquet / none / none | 4.65   | 4.65        |   -0.09%   |   2.12%    |   2.17%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q4  | parquet / none / none | 3.22   | 3.23        |   -0.26%   |   1.03%    |   1.33%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q7  | parquet / none / none | 15.84  | 15.92       |   -0.50%   |   0.91%    |   1.15%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q14 | parquet / none / none | 3.29   | 3.31        |   -0.59%   |   3.31%    |   1.58%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q22 | parquet / none / none | 2.65   | 2.67        |   -0.78%   |   3.03%    |   1.46%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q15 | parquet / none / none | 4.50   | 4.55        |   -1.19%   |   2.87%    |   2.45%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q20 | parquet / none / none | 3.84   | 3.91        |   -1.76%   |   2.20%    |   1.94%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q10 | parquet / none / none | 5.58   | 5.70        |   -2.00%   |   1.01%    |   1.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q21 | parquet / none / none | 22.84  | 23.42       |   -2.47%   |   0.68%    |   0.56%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q1  | parquet / none / none | 11.25  | 11.60       |   -3.06%   |   0.48%    |   1.74%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q12 | parquet / none / none | 3.81   | 3.98        |   -4.38%   |   1.62%    |   1.14%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q6  | parquet / none / none | 1.94   | 2.04        |   -4.85%   |   2.40%    |   1.58%        | 1           | 10    |
    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+

    +--------------------+-----------------------+---------+------------+------------+----------------+
    | Workload           | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
    +--------------------+-----------------------+---------+------------+------------+----------------+
    | TARGETED-PERF(_20) | parquet / none / none | 8.17    | -1.66%     | 2.96       | -1.48%         |
    +--------------------+-----------------------+---------+------------+------------+----------------+

    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | Workload           | Query                                                  | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | TARGETED-PERF(_20) | primitive_topn_bigint                                  | parquet / none / none | 3.32   | 2.87        |   +15.31%  | * 17.22% * |   1.64%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q4                                            | parquet / none / none | 7.07   | 6.61        |   +6.93%   | * 15.59% * |   5.08%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q1                                            | parquet / none / none | 1.18   | 1.12        |   +5.57%   |   1.94%    |   2.96%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_empty_build_join_1                           | parquet / none / none | 10.75  | 10.47       |   +2.76%   |   1.15%    |   0.94%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_top-n_all                                    | parquet / none / none | 24.30  | 23.85       |   +1.87%   |   1.40%    |   0.82%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_2                             | parquet / none / none | 2.45   | 2.42        |   +1.38%   |   1.93%    |   1.33%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_bigint_selective                      | parquet / none / none | 0.57   | 0.57        |   +1.16%   |   3.66%    |   4.19%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q3                                         | parquet / none / none | 1.70   | 1.68        |   +1.06%   |   1.88%    |   2.69%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_3                             | parquet / none / none | 4.15   | 4.13        |   +0.47%   |   1.27%    |   1.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_1                             | parquet / none / none | 1.46   | 1.46        |   +0.32%   |   1.68%    |   2.43%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_orderby_all                                  | parquet / none / none | 12.92  | 12.89       |   +0.22%   |   1.69%    |   1.12%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q4                                         | parquet / none / none | 1.68   | 1.67        |   +0.21%   |   2.31%    |   2.44%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q7                                         | parquet / none / none | 3.35   | 3.35        |   +0.14%   |   1.10%    |   1.84%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_4                          | parquet / none / none | 0.46   | 0.46        |   +0.07%   |   0.29%    |   0.27%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_pk                            | parquet / none / none | 35.99  | 36.04       |   -0.13%   |   8.55%    |   6.95%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q6                                            | parquet / none / none | 0.97   | 0.97        |   -0.16%   |   2.68%    |   2.66%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_decimal_selective                     | parquet / none / none | 0.84   | 0.84        |   -0.31%   |   2.85%    |   3.58%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_shuffle_join_union_all_with_groupby          | parquet / none / none | 20.46  | 20.53       |   -0.34%   |   0.60%    |   0.60%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q7                                            | parquet / none / none | 0.98   | 0.98        |   -0.44%   |   2.78%    |   2.33%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_exchange_shuffle                             | parquet / none / none | 25.21  | 25.34       |   -0.49%   |   1.94%    |   1.30%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_like                           | parquet / none / none | 6.27   | 6.31        |   -0.56%   |   0.59%    |   0.43%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q5                                         | parquet / none / none | 1.96   | 1.97        |   -0.60%   |   1.98%    |   1.25%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_bigint_non_selective                  | parquet / none / none | 0.53   | 0.53        |   -0.71%   |   2.64%    |   0.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_exchange_broadcast                           | parquet / none / none | 14.77  | 14.97       |   -1.33%   |   2.59%    |   2.39%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_LIMIT-Q1                                          | parquet / none / none | 0.01   | 0.01        |   -1.40%   |   3.77%    |   3.51%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_orderby_bigint                               | parquet / none / none | 2.58   | 2.61        |   -1.44%   |   2.85%    |   1.04%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q1                                         | parquet / none / none | 1.47   | 1.49        |   -1.63%   |   2.79%    |   1.62%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_decimal_non_selective                 | parquet / none / none | 0.79   | 0.81        |   -1.65%   |   1.91%    |   2.67%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q2                                         | parquet / none / none | 1.56   | 1.58        |   -1.73%   |   2.86%    |   2.57%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q6                                         | parquet / none / none | 4.48   | 4.59        |   -2.22%   |   1.21%    |   0.85%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_5                          | parquet / none / none | 11.80  | 12.10       |   -2.50%   |   3.06%    |   2.09%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_1                          | parquet / none / none | 7.96   | 8.18        |   -2.58%   |   1.69%    |   2.17%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_shuffle_join_one_to_many_string_with_groupby | parquet / none / none | 93.11  | 95.78       |   -2.78%   |   1.07%    |   1.00%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_selective                      | parquet / none / none | 0.88   | 0.91        |   -3.03%   |   4.68%    |   5.34%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q5                                            | parquet / none / none | 2.44   | 2.52        |   -3.35%   |   2.39%    |   2.04%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_highndv                       | parquet / none / none | 9.16   | 9.50        |   -3.59%   |   0.98%    |   1.56%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_3                          | parquet / none / none | 1.44   | 1.50        |   -4.14%   |   1.10%    |   1.08%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_non_selective                  | parquet / none / none | 0.89   | 0.93        |   -4.31%   |   7.20%    |   5.12%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_decimal_highndv                      | parquet / none / none | 13.98  | 14.94       |   -6.40%   | * 11.24% * |   8.66%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q2                                            | parquet / none / none | 2.87   | 3.13        |   -8.36%   |   0.57%    |   1.59%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_lowndv                        | parquet / none / none | 1.48   | 1.62        |   -8.91%   |   1.62%    |   1.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_2                          | parquet / none / none | 13.68  | 15.05       |   -9.14%   |   2.91%    |   1.83%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_decimal_lowndv.test                  | parquet / none / none | 1.47   | 1.61        |   -9.26%   |   0.19%    |   1.50%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q3                                            | parquet / none / none | 4.25   | 4.82        |   -11.82%  |   0.79%    |   0.84%        | 1           | 10    |
    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+

Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
---
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
M be/src/exec/hash-table.inline.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node.cc
5 files changed, 226 insertions(+), 251 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/4326/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4326
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 4: Code-Review+1

Carry Michael's +1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4326/2/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

PS2, Line 317: uint8_t*
> The return value has to be const if the input argument is const though. I t
I see. Makes sense.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4326

to look at the new patch set (#5).

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................

IMPALA-4008: don't bake in hash table and hash join pointers

This fixes some of the cases where memory addresses are baked into
codegen'd code.

Testing:
Ran exhaustive build.

Perf:
Ran a local perf run. No significant changes. I was able to see some
small improvements on microbenchmarks.

    +-----------+-----------------------+---------+------------+------------+----------------+
    | Workload  | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
    +-----------+-----------------------+---------+------------+------------+----------------+
    | TPCH(_20) | parquet / none / none | 9.07    | +0.46%     | 5.88       | +0.34%         |
    +-----------+-----------------------+---------+------------+------------+----------------+

    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | Workload  | Query    | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | TPCH(_20) | TPCH-Q2  | parquet / none / none | 2.12   | 1.89        |   +12.29%  | * 10.85% * | * 20.30% *     | 1           | 10    |
    | TPCH(_20) | TPCH-Q13 | parquet / none / none | 9.84   | 9.34        |   +5.39%   |   9.01%    |   3.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q17 | parquet / none / none | 14.61  | 14.19       |   +2.97%   |   2.15%    |   1.52%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q18 | parquet / none / none | 14.76  | 14.35       |   +2.82%   |   3.20%    |   2.64%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q9  | parquet / none / none | 13.72  | 13.54       |   +1.30%   |   1.75%    |   0.70%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q8  | parquet / none / none | 5.71   | 5.64        |   +1.30%   |   1.21%    |   1.23%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q19 | parquet / none / none | 47.35  | 46.75       |   +1.28%   |   2.39%    |   1.88%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q5  | parquet / none / none | 4.57   | 4.52        |   +1.20%   |   1.30%    |   0.88%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q16 | parquet / none / none | 2.07   | 2.05        |   +1.12%   |   2.59%    |   1.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q11 | parquet / none / none | 1.45   | 1.45        |   +0.15%   |   2.69%    |   2.06%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q3  | parquet / none / none | 4.65   | 4.65        |   -0.09%   |   2.12%    |   2.17%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q4  | parquet / none / none | 3.22   | 3.23        |   -0.26%   |   1.03%    |   1.33%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q7  | parquet / none / none | 15.84  | 15.92       |   -0.50%   |   0.91%    |   1.15%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q14 | parquet / none / none | 3.29   | 3.31        |   -0.59%   |   3.31%    |   1.58%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q22 | parquet / none / none | 2.65   | 2.67        |   -0.78%   |   3.03%    |   1.46%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q15 | parquet / none / none | 4.50   | 4.55        |   -1.19%   |   2.87%    |   2.45%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q20 | parquet / none / none | 3.84   | 3.91        |   -1.76%   |   2.20%    |   1.94%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q10 | parquet / none / none | 5.58   | 5.70        |   -2.00%   |   1.01%    |   1.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q21 | parquet / none / none | 22.84  | 23.42       |   -2.47%   |   0.68%    |   0.56%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q1  | parquet / none / none | 11.25  | 11.60       |   -3.06%   |   0.48%    |   1.74%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q12 | parquet / none / none | 3.81   | 3.98        |   -4.38%   |   1.62%    |   1.14%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q6  | parquet / none / none | 1.94   | 2.04        |   -4.85%   |   2.40%    |   1.58%        | 1           | 10    |
    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+

    +--------------------+-----------------------+---------+------------+------------+----------------+
    | Workload           | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
    +--------------------+-----------------------+---------+------------+------------+----------------+
    | TARGETED-PERF(_20) | parquet / none / none | 8.17    | -1.66%     | 2.96       | -1.48%         |
    +--------------------+-----------------------+---------+------------+------------+----------------+

    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | Workload           | Query                                                  | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | TARGETED-PERF(_20) | primitive_topn_bigint                                  | parquet / none / none | 3.32   | 2.87        |   +15.31%  | * 17.22% * |   1.64%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q4                                            | parquet / none / none | 7.07   | 6.61        |   +6.93%   | * 15.59% * |   5.08%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q1                                            | parquet / none / none | 1.18   | 1.12        |   +5.57%   |   1.94%    |   2.96%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_empty_build_join_1                           | parquet / none / none | 10.75  | 10.47       |   +2.76%   |   1.15%    |   0.94%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_top-n_all                                    | parquet / none / none | 24.30  | 23.85       |   +1.87%   |   1.40%    |   0.82%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_2                             | parquet / none / none | 2.45   | 2.42        |   +1.38%   |   1.93%    |   1.33%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_bigint_selective                      | parquet / none / none | 0.57   | 0.57        |   +1.16%   |   3.66%    |   4.19%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q3                                         | parquet / none / none | 1.70   | 1.68        |   +1.06%   |   1.88%    |   2.69%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_3                             | parquet / none / none | 4.15   | 4.13        |   +0.47%   |   1.27%    |   1.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_1                             | parquet / none / none | 1.46   | 1.46        |   +0.32%   |   1.68%    |   2.43%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_orderby_all                                  | parquet / none / none | 12.92  | 12.89       |   +0.22%   |   1.69%    |   1.12%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q4                                         | parquet / none / none | 1.68   | 1.67        |   +0.21%   |   2.31%    |   2.44%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q7                                         | parquet / none / none | 3.35   | 3.35        |   +0.14%   |   1.10%    |   1.84%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_4                          | parquet / none / none | 0.46   | 0.46        |   +0.07%   |   0.29%    |   0.27%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_pk                            | parquet / none / none | 35.99  | 36.04       |   -0.13%   |   8.55%    |   6.95%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q6                                            | parquet / none / none | 0.97   | 0.97        |   -0.16%   |   2.68%    |   2.66%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_decimal_selective                     | parquet / none / none | 0.84   | 0.84        |   -0.31%   |   2.85%    |   3.58%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_shuffle_join_union_all_with_groupby          | parquet / none / none | 20.46  | 20.53       |   -0.34%   |   0.60%    |   0.60%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q7                                            | parquet / none / none | 0.98   | 0.98        |   -0.44%   |   2.78%    |   2.33%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_exchange_shuffle                             | parquet / none / none | 25.21  | 25.34       |   -0.49%   |   1.94%    |   1.30%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_like                           | parquet / none / none | 6.27   | 6.31        |   -0.56%   |   0.59%    |   0.43%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q5                                         | parquet / none / none | 1.96   | 1.97        |   -0.60%   |   1.98%    |   1.25%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_bigint_non_selective                  | parquet / none / none | 0.53   | 0.53        |   -0.71%   |   2.64%    |   0.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_exchange_broadcast                           | parquet / none / none | 14.77  | 14.97       |   -1.33%   |   2.59%    |   2.39%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_LIMIT-Q1                                          | parquet / none / none | 0.01   | 0.01        |   -1.40%   |   3.77%    |   3.51%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_orderby_bigint                               | parquet / none / none | 2.58   | 2.61        |   -1.44%   |   2.85%    |   1.04%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q1                                         | parquet / none / none | 1.47   | 1.49        |   -1.63%   |   2.79%    |   1.62%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_decimal_non_selective                 | parquet / none / none | 0.79   | 0.81        |   -1.65%   |   1.91%    |   2.67%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q2                                         | parquet / none / none | 1.56   | 1.58        |   -1.73%   |   2.86%    |   2.57%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q6                                         | parquet / none / none | 4.48   | 4.59        |   -2.22%   |   1.21%    |   0.85%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_5                          | parquet / none / none | 11.80  | 12.10       |   -2.50%   |   3.06%    |   2.09%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_1                          | parquet / none / none | 7.96   | 8.18        |   -2.58%   |   1.69%    |   2.17%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_shuffle_join_one_to_many_string_with_groupby | parquet / none / none | 93.11  | 95.78       |   -2.78%   |   1.07%    |   1.00%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_selective                      | parquet / none / none | 0.88   | 0.91        |   -3.03%   |   4.68%    |   5.34%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q5                                            | parquet / none / none | 2.44   | 2.52        |   -3.35%   |   2.39%    |   2.04%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_highndv                       | parquet / none / none | 9.16   | 9.50        |   -3.59%   |   0.98%    |   1.56%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_3                          | parquet / none / none | 1.44   | 1.50        |   -4.14%   |   1.10%    |   1.08%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_non_selective                  | parquet / none / none | 0.89   | 0.93        |   -4.31%   |   7.20%    |   5.12%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_decimal_highndv                      | parquet / none / none | 13.98  | 14.94       |   -6.40%   | * 11.24% * |   8.66%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q2                                            | parquet / none / none | 2.87   | 3.13        |   -8.36%   |   0.57%    |   1.59%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_lowndv                        | parquet / none / none | 1.48   | 1.62        |   -8.91%   |   1.62%    |   1.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_2                          | parquet / none / none | 13.68  | 15.05       |   -9.14%   |   2.91%    |   1.83%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_decimal_lowndv.test                  | parquet / none / none | 1.47   | 1.61        |   -9.26%   |   0.19%    |   1.50%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q3                                            | parquet / none / none | 4.25   | 4.82        |   -11.82%  |   0.79%    |   0.84%        | 1           | 10    |
    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+

Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
---
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
M be/src/exec/hash-table.inline.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
8 files changed, 270 insertions(+), 279 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/4326/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4326
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#2).

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................

IMPALA-4008: don't bake in hash table and hash join pointers

This fixes some of the cases where memory addresses are baked into
codegen'd code.

Testing:
Ran exhaustive build.

Perf:
Ran a local perf run. No significant changes. I was able to see some
small improvements on microbenchmarks.

    +-----------+-----------------------+---------+------------+------------+----------------+
    | Workload  | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
    +-----------+-----------------------+---------+------------+------------+----------------+
    | TPCH(_20) | parquet / none / none | 9.07    | +0.46%     | 5.88       | +0.34%         |
    +-----------+-----------------------+---------+------------+------------+----------------+

    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | Workload  | Query    | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | TPCH(_20) | TPCH-Q2  | parquet / none / none | 2.12   | 1.89        |   +12.29%  | * 10.85% * | * 20.30% *     | 1           | 10    |
    | TPCH(_20) | TPCH-Q13 | parquet / none / none | 9.84   | 9.34        |   +5.39%   |   9.01%    |   3.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q17 | parquet / none / none | 14.61  | 14.19       |   +2.97%   |   2.15%    |   1.52%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q18 | parquet / none / none | 14.76  | 14.35       |   +2.82%   |   3.20%    |   2.64%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q9  | parquet / none / none | 13.72  | 13.54       |   +1.30%   |   1.75%    |   0.70%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q8  | parquet / none / none | 5.71   | 5.64        |   +1.30%   |   1.21%    |   1.23%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q19 | parquet / none / none | 47.35  | 46.75       |   +1.28%   |   2.39%    |   1.88%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q5  | parquet / none / none | 4.57   | 4.52        |   +1.20%   |   1.30%    |   0.88%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q16 | parquet / none / none | 2.07   | 2.05        |   +1.12%   |   2.59%    |   1.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q11 | parquet / none / none | 1.45   | 1.45        |   +0.15%   |   2.69%    |   2.06%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q3  | parquet / none / none | 4.65   | 4.65        |   -0.09%   |   2.12%    |   2.17%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q4  | parquet / none / none | 3.22   | 3.23        |   -0.26%   |   1.03%    |   1.33%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q7  | parquet / none / none | 15.84  | 15.92       |   -0.50%   |   0.91%    |   1.15%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q14 | parquet / none / none | 3.29   | 3.31        |   -0.59%   |   3.31%    |   1.58%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q22 | parquet / none / none | 2.65   | 2.67        |   -0.78%   |   3.03%    |   1.46%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q15 | parquet / none / none | 4.50   | 4.55        |   -1.19%   |   2.87%    |   2.45%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q20 | parquet / none / none | 3.84   | 3.91        |   -1.76%   |   2.20%    |   1.94%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q10 | parquet / none / none | 5.58   | 5.70        |   -2.00%   |   1.01%    |   1.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q21 | parquet / none / none | 22.84  | 23.42       |   -2.47%   |   0.68%    |   0.56%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q1  | parquet / none / none | 11.25  | 11.60       |   -3.06%   |   0.48%    |   1.74%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q12 | parquet / none / none | 3.81   | 3.98        |   -4.38%   |   1.62%    |   1.14%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q6  | parquet / none / none | 1.94   | 2.04        |   -4.85%   |   2.40%    |   1.58%        | 1           | 10    |
    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+

    +--------------------+-----------------------+---------+------------+------------+----------------+
    | Workload           | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
    +--------------------+-----------------------+---------+------------+------------+----------------+
    | TARGETED-PERF(_20) | parquet / none / none | 8.17    | -1.66%     | 2.96       | -1.48%         |
    +--------------------+-----------------------+---------+------------+------------+----------------+

    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | Workload           | Query                                                  | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | TARGETED-PERF(_20) | primitive_topn_bigint                                  | parquet / none / none | 3.32   | 2.87        |   +15.31%  | * 17.22% * |   1.64%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q4                                            | parquet / none / none | 7.07   | 6.61        |   +6.93%   | * 15.59% * |   5.08%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q1                                            | parquet / none / none | 1.18   | 1.12        |   +5.57%   |   1.94%    |   2.96%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_empty_build_join_1                           | parquet / none / none | 10.75  | 10.47       |   +2.76%   |   1.15%    |   0.94%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_top-n_all                                    | parquet / none / none | 24.30  | 23.85       |   +1.87%   |   1.40%    |   0.82%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_2                             | parquet / none / none | 2.45   | 2.42        |   +1.38%   |   1.93%    |   1.33%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_bigint_selective                      | parquet / none / none | 0.57   | 0.57        |   +1.16%   |   3.66%    |   4.19%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q3                                         | parquet / none / none | 1.70   | 1.68        |   +1.06%   |   1.88%    |   2.69%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_3                             | parquet / none / none | 4.15   | 4.13        |   +0.47%   |   1.27%    |   1.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_1                             | parquet / none / none | 1.46   | 1.46        |   +0.32%   |   1.68%    |   2.43%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_orderby_all                                  | parquet / none / none | 12.92  | 12.89       |   +0.22%   |   1.69%    |   1.12%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q4                                         | parquet / none / none | 1.68   | 1.67        |   +0.21%   |   2.31%    |   2.44%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q7                                         | parquet / none / none | 3.35   | 3.35        |   +0.14%   |   1.10%    |   1.84%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_4                          | parquet / none / none | 0.46   | 0.46        |   +0.07%   |   0.29%    |   0.27%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_pk                            | parquet / none / none | 35.99  | 36.04       |   -0.13%   |   8.55%    |   6.95%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q6                                            | parquet / none / none | 0.97   | 0.97        |   -0.16%   |   2.68%    |   2.66%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_decimal_selective                     | parquet / none / none | 0.84   | 0.84        |   -0.31%   |   2.85%    |   3.58%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_shuffle_join_union_all_with_groupby          | parquet / none / none | 20.46  | 20.53       |   -0.34%   |   0.60%    |   0.60%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q7                                            | parquet / none / none | 0.98   | 0.98        |   -0.44%   |   2.78%    |   2.33%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_exchange_shuffle                             | parquet / none / none | 25.21  | 25.34       |   -0.49%   |   1.94%    |   1.30%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_like                           | parquet / none / none | 6.27   | 6.31        |   -0.56%   |   0.59%    |   0.43%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q5                                         | parquet / none / none | 1.96   | 1.97        |   -0.60%   |   1.98%    |   1.25%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_bigint_non_selective                  | parquet / none / none | 0.53   | 0.53        |   -0.71%   |   2.64%    |   0.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_exchange_broadcast                           | parquet / none / none | 14.77  | 14.97       |   -1.33%   |   2.59%    |   2.39%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_LIMIT-Q1                                          | parquet / none / none | 0.01   | 0.01        |   -1.40%   |   3.77%    |   3.51%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_orderby_bigint                               | parquet / none / none | 2.58   | 2.61        |   -1.44%   |   2.85%    |   1.04%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q1                                         | parquet / none / none | 1.47   | 1.49        |   -1.63%   |   2.79%    |   1.62%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_decimal_non_selective                 | parquet / none / none | 0.79   | 0.81        |   -1.65%   |   1.91%    |   2.67%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q2                                         | parquet / none / none | 1.56   | 1.58        |   -1.73%   |   2.86%    |   2.57%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q6                                         | parquet / none / none | 4.48   | 4.59        |   -2.22%   |   1.21%    |   0.85%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_5                          | parquet / none / none | 11.80  | 12.10       |   -2.50%   |   3.06%    |   2.09%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_1                          | parquet / none / none | 7.96   | 8.18        |   -2.58%   |   1.69%    |   2.17%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_shuffle_join_one_to_many_string_with_groupby | parquet / none / none | 93.11  | 95.78       |   -2.78%   |   1.07%    |   1.00%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_selective                      | parquet / none / none | 0.88   | 0.91        |   -3.03%   |   4.68%    |   5.34%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q5                                            | parquet / none / none | 2.44   | 2.52        |   -3.35%   |   2.39%    |   2.04%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_highndv                       | parquet / none / none | 9.16   | 9.50        |   -3.59%   |   0.98%    |   1.56%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_3                          | parquet / none / none | 1.44   | 1.50        |   -4.14%   |   1.10%    |   1.08%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_non_selective                  | parquet / none / none | 0.89   | 0.93        |   -4.31%   |   7.20%    |   5.12%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_decimal_highndv                      | parquet / none / none | 13.98  | 14.94       |   -6.40%   | * 11.24% * |   8.66%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q2                                            | parquet / none / none | 2.87   | 3.13        |   -8.36%   |   0.57%    |   1.59%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_lowndv                        | parquet / none / none | 1.48   | 1.62        |   -8.91%   |   1.62%    |   1.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_2                          | parquet / none / none | 13.68  | 15.05       |   -9.14%   |   2.91%    |   1.83%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_decimal_lowndv.test                  | parquet / none / none | 1.47   | 1.61        |   -9.26%   |   0.19%    |   1.50%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q3                                            | parquet / none / none | 4.25   | 4.82        |   -11.82%  |   0.79%    |   0.84%        | 1           | 10    |
    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+

Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
---
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
M be/src/exec/hash-table.inline.h
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node.cc
5 files changed, 224 insertions(+), 239 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/4326/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4326
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4326

to look at the new patch set (#4).

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................

IMPALA-4008: don't bake in hash table and hash join pointers

This fixes some of the cases where memory addresses are baked into
codegen'd code.

Testing:
Ran exhaustive build.

Perf:
Ran a local perf run. No significant changes. I was able to see some
small improvements on microbenchmarks.

    +-----------+-----------------------+---------+------------+------------+----------------+
    | Workload  | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
    +-----------+-----------------------+---------+------------+------------+----------------+
    | TPCH(_20) | parquet / none / none | 9.07    | +0.46%     | 5.88       | +0.34%         |
    +-----------+-----------------------+---------+------------+------------+----------------+

    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | Workload  | Query    | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | TPCH(_20) | TPCH-Q2  | parquet / none / none | 2.12   | 1.89        |   +12.29%  | * 10.85% * | * 20.30% *     | 1           | 10    |
    | TPCH(_20) | TPCH-Q13 | parquet / none / none | 9.84   | 9.34        |   +5.39%   |   9.01%    |   3.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q17 | parquet / none / none | 14.61  | 14.19       |   +2.97%   |   2.15%    |   1.52%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q18 | parquet / none / none | 14.76  | 14.35       |   +2.82%   |   3.20%    |   2.64%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q9  | parquet / none / none | 13.72  | 13.54       |   +1.30%   |   1.75%    |   0.70%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q8  | parquet / none / none | 5.71   | 5.64        |   +1.30%   |   1.21%    |   1.23%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q19 | parquet / none / none | 47.35  | 46.75       |   +1.28%   |   2.39%    |   1.88%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q5  | parquet / none / none | 4.57   | 4.52        |   +1.20%   |   1.30%    |   0.88%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q16 | parquet / none / none | 2.07   | 2.05        |   +1.12%   |   2.59%    |   1.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q11 | parquet / none / none | 1.45   | 1.45        |   +0.15%   |   2.69%    |   2.06%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q3  | parquet / none / none | 4.65   | 4.65        |   -0.09%   |   2.12%    |   2.17%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q4  | parquet / none / none | 3.22   | 3.23        |   -0.26%   |   1.03%    |   1.33%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q7  | parquet / none / none | 15.84  | 15.92       |   -0.50%   |   0.91%    |   1.15%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q14 | parquet / none / none | 3.29   | 3.31        |   -0.59%   |   3.31%    |   1.58%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q22 | parquet / none / none | 2.65   | 2.67        |   -0.78%   |   3.03%    |   1.46%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q15 | parquet / none / none | 4.50   | 4.55        |   -1.19%   |   2.87%    |   2.45%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q20 | parquet / none / none | 3.84   | 3.91        |   -1.76%   |   2.20%    |   1.94%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q10 | parquet / none / none | 5.58   | 5.70        |   -2.00%   |   1.01%    |   1.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q21 | parquet / none / none | 22.84  | 23.42       |   -2.47%   |   0.68%    |   0.56%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q1  | parquet / none / none | 11.25  | 11.60       |   -3.06%   |   0.48%    |   1.74%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q12 | parquet / none / none | 3.81   | 3.98        |   -4.38%   |   1.62%    |   1.14%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q6  | parquet / none / none | 1.94   | 2.04        |   -4.85%   |   2.40%    |   1.58%        | 1           | 10    |
    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+

    +--------------------+-----------------------+---------+------------+------------+----------------+
    | Workload           | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
    +--------------------+-----------------------+---------+------------+------------+----------------+
    | TARGETED-PERF(_20) | parquet / none / none | 8.17    | -1.66%     | 2.96       | -1.48%         |
    +--------------------+-----------------------+---------+------------+------------+----------------+

    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | Workload           | Query                                                  | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | TARGETED-PERF(_20) | primitive_topn_bigint                                  | parquet / none / none | 3.32   | 2.87        |   +15.31%  | * 17.22% * |   1.64%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q4                                            | parquet / none / none | 7.07   | 6.61        |   +6.93%   | * 15.59% * |   5.08%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q1                                            | parquet / none / none | 1.18   | 1.12        |   +5.57%   |   1.94%    |   2.96%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_empty_build_join_1                           | parquet / none / none | 10.75  | 10.47       |   +2.76%   |   1.15%    |   0.94%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_top-n_all                                    | parquet / none / none | 24.30  | 23.85       |   +1.87%   |   1.40%    |   0.82%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_2                             | parquet / none / none | 2.45   | 2.42        |   +1.38%   |   1.93%    |   1.33%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_bigint_selective                      | parquet / none / none | 0.57   | 0.57        |   +1.16%   |   3.66%    |   4.19%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q3                                         | parquet / none / none | 1.70   | 1.68        |   +1.06%   |   1.88%    |   2.69%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_3                             | parquet / none / none | 4.15   | 4.13        |   +0.47%   |   1.27%    |   1.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_1                             | parquet / none / none | 1.46   | 1.46        |   +0.32%   |   1.68%    |   2.43%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_orderby_all                                  | parquet / none / none | 12.92  | 12.89       |   +0.22%   |   1.69%    |   1.12%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q4                                         | parquet / none / none | 1.68   | 1.67        |   +0.21%   |   2.31%    |   2.44%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q7                                         | parquet / none / none | 3.35   | 3.35        |   +0.14%   |   1.10%    |   1.84%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_4                          | parquet / none / none | 0.46   | 0.46        |   +0.07%   |   0.29%    |   0.27%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_pk                            | parquet / none / none | 35.99  | 36.04       |   -0.13%   |   8.55%    |   6.95%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q6                                            | parquet / none / none | 0.97   | 0.97        |   -0.16%   |   2.68%    |   2.66%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_decimal_selective                     | parquet / none / none | 0.84   | 0.84        |   -0.31%   |   2.85%    |   3.58%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_shuffle_join_union_all_with_groupby          | parquet / none / none | 20.46  | 20.53       |   -0.34%   |   0.60%    |   0.60%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q7                                            | parquet / none / none | 0.98   | 0.98        |   -0.44%   |   2.78%    |   2.33%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_exchange_shuffle                             | parquet / none / none | 25.21  | 25.34       |   -0.49%   |   1.94%    |   1.30%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_like                           | parquet / none / none | 6.27   | 6.31        |   -0.56%   |   0.59%    |   0.43%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q5                                         | parquet / none / none | 1.96   | 1.97        |   -0.60%   |   1.98%    |   1.25%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_bigint_non_selective                  | parquet / none / none | 0.53   | 0.53        |   -0.71%   |   2.64%    |   0.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_exchange_broadcast                           | parquet / none / none | 14.77  | 14.97       |   -1.33%   |   2.59%    |   2.39%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_LIMIT-Q1                                          | parquet / none / none | 0.01   | 0.01        |   -1.40%   |   3.77%    |   3.51%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_orderby_bigint                               | parquet / none / none | 2.58   | 2.61        |   -1.44%   |   2.85%    |   1.04%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q1                                         | parquet / none / none | 1.47   | 1.49        |   -1.63%   |   2.79%    |   1.62%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_decimal_non_selective                 | parquet / none / none | 0.79   | 0.81        |   -1.65%   |   1.91%    |   2.67%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q2                                         | parquet / none / none | 1.56   | 1.58        |   -1.73%   |   2.86%    |   2.57%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q6                                         | parquet / none / none | 4.48   | 4.59        |   -2.22%   |   1.21%    |   0.85%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_5                          | parquet / none / none | 11.80  | 12.10       |   -2.50%   |   3.06%    |   2.09%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_1                          | parquet / none / none | 7.96   | 8.18        |   -2.58%   |   1.69%    |   2.17%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_shuffle_join_one_to_many_string_with_groupby | parquet / none / none | 93.11  | 95.78       |   -2.78%   |   1.07%    |   1.00%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_selective                      | parquet / none / none | 0.88   | 0.91        |   -3.03%   |   4.68%    |   5.34%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q5                                            | parquet / none / none | 2.44   | 2.52        |   -3.35%   |   2.39%    |   2.04%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_highndv                       | parquet / none / none | 9.16   | 9.50        |   -3.59%   |   0.98%    |   1.56%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_3                          | parquet / none / none | 1.44   | 1.50        |   -4.14%   |   1.10%    |   1.08%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_non_selective                  | parquet / none / none | 0.89   | 0.93        |   -4.31%   |   7.20%    |   5.12%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_decimal_highndv                      | parquet / none / none | 13.98  | 14.94       |   -6.40%   | * 11.24% * |   8.66%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q2                                            | parquet / none / none | 2.87   | 3.13        |   -8.36%   |   0.57%    |   1.59%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_lowndv                        | parquet / none / none | 1.48   | 1.62        |   -8.91%   |   1.62%    |   1.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_2                          | parquet / none / none | 13.68  | 15.05       |   -9.14%   |   2.91%    |   1.83%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_decimal_lowndv.test                  | parquet / none / none | 1.47   | 1.61        |   -9.26%   |   0.19%    |   1.50%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q3                                            | parquet / none / none | 4.25   | 4.82        |   -11.82%  |   0.79%    |   0.84%        | 1           | 10    |
    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+

Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
---
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
M be/src/exec/hash-table.inline.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
8 files changed, 245 insertions(+), 265 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/26/4326/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4326
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 3: Code-Review+1

Other the formatting problem which I find quite annoying, the code change looks good.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 6: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 3:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/4326/3/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:

PS3, Line 300: ExprValuesHash
> CurExprValuesHash()  [see below for motiviation]
Done


PS3, Line 303: SetExprValuesHash
> Let's rename this to more closely match cur_expr_values() rather than match
Done


PS3, Line 309: cur_expr_values_null
> comment explaining that there is one byte per null value (i.e. these are us
Done.

I'm not actually sure why we need this for codegen, since bool and uint8_t should both be represented as int8 internally in LLVM, but I don't want to get distracted digging into that, so I left a TODO.

There's some weirdness with vector<bool> where it uses bitfields but otherwise C++ bools should be 8 bits - maybe that was the reason.


PS3, Line 403: in
> the first time I read this, I read it as this functions stores the values t
Makes sense - done.


PS3, Line 403: in
> same
Done


PS3, Line 413: in
> same
Done


PS3, Line 431: This will be replaced by
             :   /// codegen.
> nit: move this sentence to end of the comment so it's standardized with sim
Done


PS3, Line 438: with 'cur_expr_values_'
> "to compare against the current row."  (since it also uses cur_expr_values_
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4326/2/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

Line 185: uint32_t HashTableCtx::HashVariableLenRow(
> Can we configure clang-format to not format the code this way ? I find such
It doesn't look like there is an option to do that in this case. There is a penalty option for function calls but not function declarations. I tried tweaking a couple of related things but it looks like this is the behaviour you get when BinPackParameters is true.

http://llvm.org/releases/3.8.0/tools/clang/docs/ClangFormatStyleOptions.html


PS2, Line 317: uint8_t*
> Hmm...I think I meant the first argument, not the return value.
The return value has to be const if the input argument is const though. I think the only way to solve it in C++ is to add overloads, but it doesn't seem worth it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4326/2/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

Line 185: uint32_t HashTableCtx::HashVariableLenRow(
> The whitespace decisions were made by clang-format. It does seem a little d
Can we configure clang-format to not format the code this way ? I find such inconsistency very annoying but that's just me.


PS2, Line 317: uint8_t*
> Some of the callsites need it to return a non-const pointer, e.g. EvalRow()
Hmm...I think I meant the first argument, not the return value.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 2:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/4326/2/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

Line 185: uint32_t HashTableCtx::HashVariableLenRow(
nit: this new line seems to make the formatting a bit inconsistent with say line 163. Same for other places in this file.


PS2, Line 317: uint8_t*
const uint8_t*


PS2, Line 322: ExprValueNullPtr(
This function seems to have only one caller (ExprValueNull()) left. I find the asymmetry between ExprValuePtr() and ExprValueNullPtr() misleading. How about inlining the logic directly into the only caller ?


Line 967:     return Status(
Wondering why the extra new line here ?


http://gerrit.cloudera.org:8080/#/c/4326/2/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:

Line 158:     return expr_values_cache_.ExprValuePtr(expr_values_cache_.cur_expr_values(), expr_idx);
nit: long line


PS2, Line 406: i
missing space ?


PS2, Line 407:  
extra space


Line 419:   bool IR_NO_INLINE EvalProbeRow(
nit: this extra new line seems a bit inconsistent with the rest of the files (e.g. line 432).


PS2, Line 425: rows
a row ?


Line 429:   /// 'expr_values_null'
Extra new line again. Why not continue the next line here ?


PS2, Line 436:  
nit: extra space


PS2, Line 446: IR_ALWAYS_INLINE 
ALWAYS_INLINE ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 4:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4326/4/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:

Line 308:     uint8_t* ALWAYS_INLINE cur_expr_values() const { return cur_expr_values_; }
let's add a short comment here too:
/// Returns the current row's expression buffer. 
or something.


PS4, Line 310: .
for the current row.


PS4, Line 400: into
This one should have stayed "in" (or "of"), right? i.e. it doesn't write *expr_values.
(Could make them 'const uint8_t*' to clarify that too).


Line 427:   uint32_t HashVariableLenRow(uint8_t* expr_values, uint8_t* expr_values_null) const;
how about making these const as well.


Line 441:       TupleRow* build_row, uint8_t* expr_values, uint8_t* expr_values_null) const;
these too could be const to clarify they are "in" params.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


Patch Set 6: Code-Review+2

rebase

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4008: don't bake in hash table and hash join pointers

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4008: don't bake in hash table and hash join pointers
......................................................................


IMPALA-4008: don't bake in hash table and hash join pointers

This fixes some of the cases where memory addresses are baked into
codegen'd code.

Testing:
Ran exhaustive build.

Perf:
Ran a local perf run. No significant changes. I was able to see some
small improvements on microbenchmarks.

    +-----------+-----------------------+---------+------------+------------+----------------+
    | Workload  | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
    +-----------+-----------------------+---------+------------+------------+----------------+
    | TPCH(_20) | parquet / none / none | 9.07    | +0.46%     | 5.88       | +0.34%         |
    +-----------+-----------------------+---------+------------+------------+----------------+

    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | Workload  | Query    | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | TPCH(_20) | TPCH-Q2  | parquet / none / none | 2.12   | 1.89        |   +12.29%  | * 10.85% * | * 20.30% *     | 1           | 10    |
    | TPCH(_20) | TPCH-Q13 | parquet / none / none | 9.84   | 9.34        |   +5.39%   |   9.01%    |   3.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q17 | parquet / none / none | 14.61  | 14.19       |   +2.97%   |   2.15%    |   1.52%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q18 | parquet / none / none | 14.76  | 14.35       |   +2.82%   |   3.20%    |   2.64%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q9  | parquet / none / none | 13.72  | 13.54       |   +1.30%   |   1.75%    |   0.70%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q8  | parquet / none / none | 5.71   | 5.64        |   +1.30%   |   1.21%    |   1.23%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q19 | parquet / none / none | 47.35  | 46.75       |   +1.28%   |   2.39%    |   1.88%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q5  | parquet / none / none | 4.57   | 4.52        |   +1.20%   |   1.30%    |   0.88%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q16 | parquet / none / none | 2.07   | 2.05        |   +1.12%   |   2.59%    |   1.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q11 | parquet / none / none | 1.45   | 1.45        |   +0.15%   |   2.69%    |   2.06%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q3  | parquet / none / none | 4.65   | 4.65        |   -0.09%   |   2.12%    |   2.17%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q4  | parquet / none / none | 3.22   | 3.23        |   -0.26%   |   1.03%    |   1.33%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q7  | parquet / none / none | 15.84  | 15.92       |   -0.50%   |   0.91%    |   1.15%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q14 | parquet / none / none | 3.29   | 3.31        |   -0.59%   |   3.31%    |   1.58%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q22 | parquet / none / none | 2.65   | 2.67        |   -0.78%   |   3.03%    |   1.46%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q15 | parquet / none / none | 4.50   | 4.55        |   -1.19%   |   2.87%    |   2.45%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q20 | parquet / none / none | 3.84   | 3.91        |   -1.76%   |   2.20%    |   1.94%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q10 | parquet / none / none | 5.58   | 5.70        |   -2.00%   |   1.01%    |   1.79%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q21 | parquet / none / none | 22.84  | 23.42       |   -2.47%   |   0.68%    |   0.56%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q1  | parquet / none / none | 11.25  | 11.60       |   -3.06%   |   0.48%    |   1.74%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q12 | parquet / none / none | 3.81   | 3.98        |   -4.38%   |   1.62%    |   1.14%        | 1           | 10    |
    | TPCH(_20) | TPCH-Q6  | parquet / none / none | 1.94   | 2.04        |   -4.85%   |   2.40%    |   1.58%        | 1           | 10    |
    +-----------+----------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+

    +--------------------+-----------------------+---------+------------+------------+----------------+
    | Workload           | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
    +--------------------+-----------------------+---------+------------+------------+----------------+
    | TARGETED-PERF(_20) | parquet / none / none | 8.17    | -1.66%     | 2.96       | -1.48%         |
    +--------------------+-----------------------+---------+------------+------------+----------------+

    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | Workload           | Query                                                  | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
    | TARGETED-PERF(_20) | primitive_topn_bigint                                  | parquet / none / none | 3.32   | 2.87        |   +15.31%  | * 17.22% * |   1.64%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q4                                            | parquet / none / none | 7.07   | 6.61        |   +6.93%   | * 15.59% * |   5.08%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q1                                            | parquet / none / none | 1.18   | 1.12        |   +5.57%   |   1.94%    |   2.96%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_empty_build_join_1                           | parquet / none / none | 10.75  | 10.47       |   +2.76%   |   1.15%    |   0.94%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_top-n_all                                    | parquet / none / none | 24.30  | 23.85       |   +1.87%   |   1.40%    |   0.82%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_2                             | parquet / none / none | 2.45   | 2.42        |   +1.38%   |   1.93%    |   1.33%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_bigint_selective                      | parquet / none / none | 0.57   | 0.57        |   +1.16%   |   3.66%    |   4.19%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q3                                         | parquet / none / none | 1.70   | 1.68        |   +1.06%   |   1.88%    |   2.69%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_3                             | parquet / none / none | 4.15   | 4.13        |   +0.47%   |   1.27%    |   1.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_broadcast_join_1                             | parquet / none / none | 1.46   | 1.46        |   +0.32%   |   1.68%    |   2.43%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_orderby_all                                  | parquet / none / none | 12.92  | 12.89       |   +0.22%   |   1.69%    |   1.12%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q4                                         | parquet / none / none | 1.68   | 1.67        |   +0.21%   |   2.31%    |   2.44%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q7                                         | parquet / none / none | 3.35   | 3.35        |   +0.14%   |   1.10%    |   1.84%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_4                          | parquet / none / none | 0.46   | 0.46        |   +0.07%   |   0.29%    |   0.27%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_pk                            | parquet / none / none | 35.99  | 36.04       |   -0.13%   |   8.55%    |   6.95%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q6                                            | parquet / none / none | 0.97   | 0.97        |   -0.16%   |   2.68%    |   2.66%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_decimal_selective                     | parquet / none / none | 0.84   | 0.84        |   -0.31%   |   2.85%    |   3.58%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_shuffle_join_union_all_with_groupby          | parquet / none / none | 20.46  | 20.53       |   -0.34%   |   0.60%    |   0.60%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q7                                            | parquet / none / none | 0.98   | 0.98        |   -0.44%   |   2.78%    |   2.33%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_exchange_shuffle                             | parquet / none / none | 25.21  | 25.34       |   -0.49%   |   1.94%    |   1.30%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_like                           | parquet / none / none | 6.27   | 6.31        |   -0.56%   |   0.59%    |   0.43%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q5                                         | parquet / none / none | 1.96   | 1.97        |   -0.60%   |   1.98%    |   1.25%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_bigint_non_selective                  | parquet / none / none | 0.53   | 0.53        |   -0.71%   |   2.64%    |   0.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_exchange_broadcast                           | parquet / none / none | 14.77  | 14.97       |   -1.33%   |   2.59%    |   2.39%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_LIMIT-Q1                                          | parquet / none / none | 0.01   | 0.01        |   -1.40%   |   3.77%    |   3.51%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_orderby_bigint                               | parquet / none / none | 2.58   | 2.61        |   -1.44%   |   2.85%    |   1.04%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q1                                         | parquet / none / none | 1.47   | 1.49        |   -1.63%   |   2.79%    |   1.62%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_decimal_non_selective                 | parquet / none / none | 0.79   | 0.81        |   -1.65%   |   1.91%    |   2.67%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q2                                         | parquet / none / none | 1.56   | 1.58        |   -1.73%   |   2.86%    |   2.57%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_STRING-Q6                                         | parquet / none / none | 4.48   | 4.59        |   -2.22%   |   1.21%    |   0.85%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_5                          | parquet / none / none | 11.80  | 12.10       |   -2.50%   |   3.06%    |   2.09%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_1                          | parquet / none / none | 7.96   | 8.18        |   -2.58%   |   1.69%    |   2.17%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_shuffle_join_one_to_many_string_with_groupby | parquet / none / none | 93.11  | 95.78       |   -2.78%   |   1.07%    |   1.00%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_selective                      | parquet / none / none | 0.88   | 0.91        |   -3.03%   |   4.68%    |   5.34%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q5                                            | parquet / none / none | 2.44   | 2.52        |   -3.35%   |   2.39%    |   2.04%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_highndv                       | parquet / none / none | 9.16   | 9.50        |   -3.59%   |   0.98%    |   1.56%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_3                          | parquet / none / none | 1.44   | 1.50        |   -4.14%   |   1.10%    |   1.08%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_filter_string_non_selective                  | parquet / none / none | 0.89   | 0.93        |   -4.31%   |   7.20%    |   5.12%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_decimal_highndv                      | parquet / none / none | 13.98  | 14.94       |   -6.40%   | * 11.24% * |   8.66%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q2                                            | parquet / none / none | 2.87   | 3.13        |   -8.36%   |   0.57%    |   1.59%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_bigint_lowndv                        | parquet / none / none | 1.48   | 1.62        |   -8.91%   |   1.62%    |   1.28%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_conjunct_ordering_2                          | parquet / none / none | 13.68  | 15.05       |   -9.14%   |   2.91%    |   1.83%        | 1           | 10    |
    | TARGETED-PERF(_20) | primitive_groupby_decimal_lowndv.test                  | parquet / none / none | 1.47   | 1.61        |   -9.26%   |   0.19%    |   1.50%        | 1           | 10    |
    | TARGETED-PERF(_20) | PERF_AGG-Q3                                            | parquet / none / none | 4.25   | 4.82        |   -11.82%  |   0.79%    |   0.84%        | 1           | 10    |
    +--------------------+--------------------------------------------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+

Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Reviewed-on: http://gerrit.cloudera.org:8080/4326
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hash-table.h
M be/src/exec/hash-table.inline.h
M be/src/exec/partitioned-aggregation-node-ir.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node-ir.cc
M be/src/exec/partitioned-hash-join-node.cc
8 files changed, 270 insertions(+), 279 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Tim Armstrong: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie353666dbb5c958f0094d169306fe930ec3014c5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>