Hello Lars Volker, 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 (#6). 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, 64 insertions(+), 53 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/85/8085/6 -- 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: 6 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com>