Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-3210: last/first_value() support for IGNORE NULLS ......................................................................
Patch Set 5: I ran a performance test with every combination of start and end bounds to see what the effects of the changes to 'standardize' would have: +-----------------+---------------------+-----------------------+--------+-------------+------------+-----------+----------------+-------------+-------+ | Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Num Clients | Iters | +-----------------+---------------------+-----------------------+--------+-------------+------------+-----------+----------------+-------------+-------+ | TARGETED-PERF() | PERF_FIRST_VALUE_5 | parquet / none / none | 30.55 | 28.65 | +6.63% | 0.31% | 0.22% | 1 | 3 | | TARGETED-PERF() | PERF_FIRST_VALUE_7 | parquet / none / none | 30.13 | 28.34 | +6.28% | 0.02% | 0.36% | 1 | 3 | | TARGETED-PERF() | PERF_FIRST_VALUE_6 | parquet / none / none | 30.44 | 28.68 | +6.15% | 0.40% | 0.28% | 1 | 3 | | TARGETED-PERF() | PERF_FIRST_VALUE_2 | parquet / none / none | 28.25 | 27.87 | +1.35% | 0.14% | 0.08% | 1 | 3 | | TARGETED-PERF() | PERF_FIRST_VALUE_1 | parquet / none / none | 28.20 | 27.85 | +1.23% | 0.35% | 0.25% | 1 | 3 | | TARGETED-PERF() | PERF_FIRST_VALUE_3 | parquet / none / none | 28.36 | 28.07 | +1.01% | 0.21% | 0.80% | 1 | 3 | | TARGETED-PERF() | PERF_FIRST_VALUE_10 | parquet / none / none | 30.54 | 30.37 | +0.56% | 0.12% | 0.16% | 1 | 3 | | TARGETED-PERF() | PERF_LAST_VALUE_7 | parquet / none / none | 30.64 | 30.56 | +0.28% | 0.07% | 0.31% | 1 | 3 | | TARGETED-PERF() | PERF_LAST_VALUE_13 | parquet / none / none | 28.35 | 28.39 | -0.14% | 0.27% | 0.14% | 1 | 3 | | TARGETED-PERF() | PERF_LAST_VALUE_5 | parquet / none / none | 30.10 | 30.16 | -0.19% | 0.12% | 0.14% | 1 | 3 | | TARGETED-PERF() | PERF_LAST_VALUE_8 | parquet / none / none | 28.42 | 28.48 | -0.20% | 0.02% | 0.08% | 1 | 3 | | TARGETED-PERF() | PERF_LAST_VALUE_2 | parquet / none / none | 27.76 | 27.83 | -0.24% | 0.39% | 0.30% | 1 | 3 | | TARGETED-PERF() | PERF_LAST_VALUE_3 | parquet / none / none | 27.87 | 27.97 | -0.36% | 0.45% | 0.24% | 1 | 3 | | TARGETED-PERF() | PERF_LAST_VALUE_4 | parquet / none / none | 25.69 | 25.78 | -0.37% | 0.08% | 0.20% | 1 | 3 | | TARGETED-PERF() | PERF_LAST_VALUE_9 | parquet / none / none | 30.47 | 30.61 | -0.46% | 0.05% | 0.14% | 1 | 3 | | TARGETED-PERF() | PERF_LAST_VALUE_6 | parquet / none / none | 29.84 | 29.99 | -0.51% | 0.47% | 0.49% | 1 | 3 | | TARGETED-PERF() | PERF_FIRST_VALUE_9 | parquet / none / none | 30.06 | 30.30 | -0.80% | 0.32% | 0.13% | 1 | 3 | | TARGETED-PERF() | PERF_FIRST_VALUE_12 | parquet / none / none | 29.92 | 30.27 | -1.16% | 0.22% | 0.20% | 1 | 3 | | TARGETED-PERF() | PERF_LAST_VALUE_10 | parquet / none / none | 30.13 | 30.54 | -1.35% | 0.40% | 0.17% | 1 | 3 | | TARGETED-PERF() | PERF_FIRST_VALUE_8 | parquet / none / none | 27.84 | 28.30 | -1.62% | 0.05% | 0.48% | 1 | 3 | | TARGETED-PERF() | PERF_LAST_VALUE_12 | parquet / none / none | 30.09 | 30.60 | -1.65% | 0.15% | 0.39% | 1 | 3 | | TARGETED-PERF() | PERF_LAST_VALUE_11 | parquet / none / none | 27.92 | 28.40 | -1.70% | 0.20% | 0.15% | 1 | 3 | | TARGETED-PERF() | PERF_LAST_VALUE_1 | parquet / none / none | 27.77 | 28.59 | -2.87% | 0.14% | 0.26% | 1 | 3 | | TARGETED-PERF() | PERF_FIRST_VALUE_11 | parquet / none / none | 28.29 | 30.22 | -6.40% | 0.29% | 0.07% | 1 | 3 | | TARGETED-PERF() | PERF_FIRST_VALUE_13 | parquet / none / none | 27.75 | 30.24 | -8.21% | 0.11% | 0.07% | 1 | 3 | | TARGETED-PERF() | PERF_FIRST_VALUE_4 | parquet / none / none | 25.61 | 28.34 | -9.65% | 0.13% | 0.06% | 1 | 3 | +-----------------+---------------------+-----------------------+--------+-------------+------------+-----------+----------------+-------------+-------+ last_value is basically unaffected There are 3 first_value queries that got slower - 5. 6, and 7 in the chart. These queries used to use first_value_rewrite, but now use last_value with a reversed window for simplicity. Fortunately, the slowdown is small. Unfortunately, these are probably some of the more common first_value bounds - x preceding to y preceding, current row, or y following - since a start bound for first_value of unbounded preceding (always produces the first row) or current row (always produces the current row) aren't very useful. Unexpectedly, there were a few first_value queries that actually got faster: FIRST_VALUE_4: first_value(unbounded preceding to unbounded following) - we used to rewrite it to first_value(unbounded preceding to current row), which caused us to call GetNext on every row. After this change, it doesn't get rewritten, so the entire input gets consumed before calling GetNext once. FIRST_VALUE_11: first_value(current row, unbounded following) - we used to rewrite it to last_value(current row, current row), which required calling Add, Remove, and Get on every row. Now we rewrite it as last_value(unbounded preceding, current row) (with a flipped window), which never has to call Remove. FIRST_VALUE_13: first_value(x following, unbounded following) - basically the same as 11, we used to rewrite it as last_value(x following, x following), which required calling Add, Remove, and Get on every row. Now we rewrite it as last_value(unbounded preceding, x preceding) (with a flipped window), which never has to call Remove As has been pointed out in the comments, its not clear what the ultimate performance effect will be, since these changes also affect when these functions can be grouped. It really depends on the workload. But at the very least it looks like eliminating first_value_rewrite isn't that much of a performance hit compared to how much it simplifies the code. -- To view, visit http://gerrit.cloudera.org:8080/3328 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic27525e2237fb54318549d2674f1610884208e9b Gerrit-PatchSet: 5 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: No
