[Impala-ASF-CR] IMPALA-5307: part 1: don't transfer disk I/O buffers out of parquet

["Tim Armstrong (Code Review)" <ge...@cloudera.org>]

Hello Lars Volker, Alex Behm, Dan Hecht, 

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

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

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

Change subject: IMPALA-5307: part 1: don't transfer disk I/O buffers out of parquet
......................................................................

IMPALA-5307: part 1: don't transfer disk I/O buffers out of parquet

This change only affects uncompressed plain-encoded Parquet where
RowBatches may directly reference strings stored in the I/O
buffers. The proposed fix is to simply copy the data pages if
needed then use the same logic that we use for decompressed data
pages.

This copy inevitably adds some CPU overhead, but I believe this is
acceptable because:
* We generally recommend using compression, and optimize for that
  case.
* Copying memory is cheaper than decompressing data.
* Scans of uncompressed data are very likely to be I/O bound.

This allows several major simplifications:
* The resource management for compressed and uncompressed
  scans is much more similar.
* We don't need to attach Disk I/O buffers to RowBatches.
* We don't need to deal with attaching I/O buffers in
  ScannerContext.
* Column readers can release each I/O buffer *before* advancing to
  the next one, making it easier to reason about resource
  consumption. E.g. each Parquet column only needs one I/O buffer at
  a time to make progress.

Future changes will apply to Avro, Sequence Files and Text. Once
all scanners are converted, ScannerContext::contains_tuple_data_
will always be false and we can remove some dead code.

Testing
=======
Ran core ASAN and exhaustive debug builds.

Perf
====
No difference in most cases when scanning uncompressed parquet.
There is a significant regression (50% increase in runtime) in
targeted perf tests scanning non-dictionary-encoded strings (see
benchmark output below).  After the regression performance is
comparable to Snappy compression.

I also did a TPC-H run but ran into some issues with the report
generator. I manually compared times and there were no regressions.

+--------------------+-----------------------+---------+------------+------------+----------------+
| Workload           | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+--------------------+-----------------------+---------+------------+------------+----------------+
| TARGETED-PERF(_61) | parquet / none / none | 23.02   | +0.60%     | 4.23       | +5.97%         |
+--------------------+-----------------------+---------+------------+------------+----------------+

+--------------------+--------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
| Workload           | Query              | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%)  | Base StdDev(%) | Num Clients | Iters |
+--------------------+--------------------+-----------------------+--------+-------------+------------+------------+----------------+-------------+-------+
| TARGETED-PERF(_61) | PERF_STRING-Q2     | parquet / none / none | 3.00   | 1.98        | R +52.10%  |   0.97%    |   1.25%        | 1           | 5     |
| TARGETED-PERF(_61) | PERF_STRING-Q1     | parquet / none / none | 2.86   | 1.92        | R +49.11%  |   0.34%    |   2.34%        | 1           | 5     |
| TARGETED-PERF(_61) | PERF_STRING-Q3     | parquet / none / none | 3.16   | 2.15        | R +47.04%  |   1.03%    |   0.72%        | 1           | 5     |
| TARGETED-PERF(_61) | PERF_STRING-Q4     | parquet / none / none | 3.16   | 2.17        | R +45.60%  |   0.14%    |   1.11%        | 1           | 5     |
| TARGETED-PERF(_61) | PERF_STRING-Q5     | parquet / none / none | 3.51   | 2.55        | R +37.88%  |   0.83%    |   0.49%        | 1           | 5     |
| TARGETED-PERF(_61) | PERF_AGG-Q5        | parquet / none / none | 0.79   | 0.61        | R +30.86%  |   1.54%    |   4.10%        | 1           | 5     |
| TARGETED-PERF(_61) | primitive_top-n_al | parquet / none / none | 39.45  | 35.07       |   +12.51%  |   0.29%    |   0.29%        | 1           | 5     |
| TARGETED-PERF(_61) | PERF_STRING-Q7     | parquet / none / none | 6.78   | 6.10        |   +11.13%  |   0.99%    |   0.74%        | 1           | 5     |
| TARGETED-PERF(_61) | PERF_STRING-Q6     | parquet / none / none | 8.83   | 8.14        |   +8.52%   |   0.15%    |   0.32%        | 1           | 5     |
...

Change-Id: I767c1e2dabde7d5bd7a4d5c1ec6d14801b8260d2
---
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/parquet-column-readers.cc
M be/src/exec/parquet-column-readers.h
M be/src/exec/scanner-context.h
4 files changed, 66 insertions(+), 53 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/8085/7
-- 
To view, visit http://gerrit.cloudera.org:8080/8085
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I767c1e2dabde7d5bd7a4d5c1ec6d14801b8260d2
Gerrit-Change-Number: 8085
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>