You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2019/03/20 15:56:43 UTC
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12797
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Based on initial draft patch by Pooja Nilangekar.
Codegen'd expressions can be executed in two ways - either by
being called directly from a fully codegend function, or from
interpreted code via a function pointer (previously
ScalarFnCall::scalar_fn_wrapper_).
This change moves the function pointer from ScalarFnCall to its
base class ScalarExpr, so the full expr tree can be codegen'd, not
just the ScalarFnCall subtrees. The key refactoring and improvements
are:
* ScalarExpr::Get*Val() switches between interpreted and the codegen'd
function pointer code paths in an inline function, avoiding a
virtual function call to ScalarFnCal::Get*Val().
* Boilerplate logic is moved to ScalarExpr::GetCodegendComputeFn(),
which calls a virtual function GetCodegenComputeFnImpl().
* ScalarFnCall's logic for deciding whether to interpret or codegen is
better abstracted and exposed to ScalarExpr as IsInterpretable()
and ShouldCodegen() methods.
* The ScalarExpr::codegend_compute_fn_ function pointer is only
populated for expressions that are "codgen entry points". The
include the roots of expr trees and non-root expressions
where the parent expression calls Get*Val() from the
pseudo-codegend GetCodegendComputeFnWrapper().
* ScalarFnCall is always initialised for interpreted execution.
Othe the function pointer is needed for non-root expressions,
e.g. to support ScalarExprEvaluator::GetConstantVal().
* Latent bugs/gaps for codegen of CollectionVal are fixed. CollectionVal
is modified to use the StringVal memory layout to allow code sharing
with StringVal. These fixes allowed simplification of
IsNotEmptyPredicate codegen (from IMPALA-7657).
IMPALA-7331 (CHAR codegen support functions) is also fixed because
it was simpler to enable CHAR codegen within ScalarExpr than to carry
forward the exiting CHAR workarounds from ScalarFnCall. The CHAR-specific
codegen support required in the scalar expr subsystem is very limited.
StringVal intermediates are used everywhere. Only SlotRef actually
operates on the different tuple layout, and the required codegen support
for SlotRef already exists for UDA intermediates anyway.
Testing:
* Ran exhaustive tests.
Perf:
* Ran a basic insert benchmark, which went from 10.1s to 7.6s
create table foo stored as parquet as
select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end
from tpch30_parquet.lineitem;
* Added perf regression test to tpcds-insert, similar to the manual
benchmark.
* Ran single-node TPC-H with large and small scale factors, to estimate
impact on execution perf and query startup time, respectively.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 6.84 | -0.18% | 4.49 | -0.31% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.58 | 2.47 | +4.18% | 1.29% | 0.88% | 5 | +4.12% | 2.31 | 5.81 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 4.81 | 4.61 | +4.33% | 2.18% | 2.15% | 5 | +3.91% | 1.73 | 3.09 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 26.45 | 26.16 | +1.09% | 0.37% | 0.50% | 5 | +1.36% | 2.02 | 3.94 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 15.92 | 15.75 | +1.09% | 2.87% | 1.65% | 5 | +0.88% | 0.29 | 0.73 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.38 | 2.35 | +1.12% | 1.64% | 1.11% | 5 | +0.80% | 1.15 | 1.26 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.13% | 7.68% | 5.37% | 5 | -0.34% | -0.29 | 0.27 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 18.10 | 18.02 | +0.42% | 2.70% | 0.56% | 5 | +0.28% | 0.29 | 0.34 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 4.72 | 4.72 | -0.04% | 1.20% | 1.65% | 5 | +0.05% | 0.00 | -0.04 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.92 | 3.93 | -0.26% | 1.08% | 2.36% | 5 | +0.20% | 0.58 | -0.23 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.27 | 1.27 | -0.28% | 0.22% | 0.88% | 5 | +0.09% | 0.29 | -0.68 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.64 | 2.65 | -0.45% | 1.65% | 0.65% | 5 | -0.24% | -0.58 | -0.57 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 3.10 | 3.13 | -0.76% | 1.47% | 1.12% | 5 | -0.21% | -0.29 | -0.93 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 1.20 | 1.21 | -0.80% | 2.26% | 2.47% | 5 | -0.82% | -1.15 | -0.53 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 1.97 | 1.99 | -1.37% | 1.84% | 3.21% | 5 | -0.47% | -0.58 | -0.83 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 11.53 | 11.63 | -0.91% | 0.46% | 0.49% | 5 | -0.95% | -2.02 | -3.08 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 5.13 | 5.21 | -1.51% | 2.24% | 4.05% | 5 | -0.94% | -0.58 | -0.73 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 3.61 | 3.66 | -1.40% | 0.66% | 0.79% | 5 | -1.33% | -1.73 | -3.05 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 19.42 | 19.71 | -1.52% | 1.34% | 1.39% | 5 | -1.22% | -1.44 | -1.76 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 5.08 | 5.15 | -1.49% | 1.34% | 0.73% | 5 | -1.35% | -1.44 | -2.20 |
| TPCH(30) | TPCH-Q15 | parquet / none / none | 3.42 | 3.49 | -1.92% | 0.93% | 1.47% | 5 | -1.53% | -1.15 | -2.49 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.15 | 1.19 | -3.17% | 2.27% | 1.95% | 5 | -4.21% | -1.15 | -2.41 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 9.26 | 9.63 | -3.85% | 0.62% | 0.59% | 5 | -3.78% | -2.31 | -10.25 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
Cluster Name: UNKNOWN
Lab Run Info: UNKNOWN
Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE ()
Baseline Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE (2019-03-19)
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(2) | parquet / none / none | 0.90 | -0.08% | 0.80 | -0.05% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(2) | TPCH-Q18 | parquet / none / none | 1.22 | 1.19 | +1.93% | 3.81% | 4.46% | 20 | +3.34% | 1.62 | 1.46 |
| TPCH(2) | TPCH-Q10 | parquet / none / none | 0.74 | 0.73 | +1.97% | 3.36% | 2.94% | 20 | +0.97% | 1.88 | 1.95 |
| TPCH(2) | TPCH-Q11 | parquet / none / none | 0.49 | 0.48 | +1.91% | 6.19% | 4.64% | 20 | +0.25% | 0.95 | 1.09 |
| TPCH(2) | TPCH-Q4 | parquet / none / none | 0.43 | 0.43 | +1.99% | 6.26% | 5.86% | 20 | +0.15% | 0.92 | 1.03 |
| TPCH(2) | TPCH-Q15 | parquet / none / none | 0.50 | 0.49 | +1.82% | 7.32% | 6.35% | 20 | +0.26% | 1.01 | 0.83 |
| TPCH(2) | TPCH-Q1 | parquet / none / none | 0.98 | 0.97 | +0.79% | 4.64% | 2.73% | 20 | +0.36% | 0.77 | 0.65 |
| TPCH(2) | TPCH-Q19 | parquet / none / none | 0.83 | 0.83 | +0.65% | 3.33% | 2.80% | 20 | +0.44% | 2.18 | 0.67 |
| TPCH(2) | TPCH-Q14 | parquet / none / none | 0.62 | 0.62 | +0.97% | 2.86% | 1.00% | 20 | +0.04% | 0.13 | 1.42 |
| TPCH(2) | TPCH-Q3 | parquet / none / none | 0.88 | 0.87 | +0.57% | 2.17% | 1.74% | 20 | +0.29% | 1.15 | 0.92 |
| TPCH(2) | TPCH-Q12 | parquet / none / none | 0.53 | 0.53 | +0.27% | 4.58% | 5.78% | 20 | +0.46% | 1.47 | 0.16 |
| TPCH(2) | TPCH-Q17 | parquet / none / none | 0.72 | 0.72 | +0.15% | 3.64% | 5.55% | 20 | +0.21% | 0.86 | 0.10 |
| TPCH(2) | TPCH-Q21 | parquet / none / none | 2.05 | 2.05 | +0.21% | 1.99% | 2.37% | 20 | +0.01% | 0.25 | 0.30 |
| TPCH(2) | TPCH-Q5 | parquet / none / none | 1.28 | 1.27 | +0.24% | 1.61% | 1.80% | 20 | -0.02% | -0.57 | 0.44 |
| TPCH(2) | TPCH-Q13 | parquet / none / none | 1.27 | 1.27 | -0.34% | 1.69% | 1.83% | 20 | -0.20% | -1.65 | -0.61 |
| TPCH(2) | TPCH-Q7 | parquet / none / none | 1.72 | 1.73 | -0.55% | 2.40% | 1.69% | 20 | -0.03% | -0.42 | -0.83 |
| TPCH(2) | TPCH-Q8 | parquet / none / none | 1.27 | 1.28 | -0.68% | 3.10% | 3.89% | 20 | -0.06% | -0.54 | -0.62 |
| TPCH(2) | TPCH-Q6 | parquet / none / none | 0.36 | 0.36 | -0.84% | 0.79% | 3.51% | 20 | -0.07% | -0.36 | -1.04 |
| TPCH(2) | TPCH-Q2 | parquet / none / none | 0.65 | 0.65 | -1.17% | 4.76% | 5.99% | 20 | -0.05% | -0.25 | -0.69 |
| TPCH(2) | TPCH-Q9 | parquet / none / none | 1.59 | 1.62 | -2.01% | 1.45% | 5.12% | 20 | -0.16% | -1.24 | -1.69 |
| TPCH(2) | TPCH-Q20 | parquet / none / none | 0.68 | 0.69 | -1.73% | 4.35% | 4.43% | 20 | -0.49% | -1.74 | -1.25 |
| TPCH(2) | TPCH-Q22 | parquet / none / none | 0.38 | 0.40 | -2.89% | 7.42% | 6.39% | 20 | -0.21% | -0.66 | -1.34 |
| TPCH(2) | TPCH-Q16 | parquet / none / none | 0.59 | 0.62 | -4.01% | 6.33% | 5.83% | 20 | -4.72% | -1.39 | -2.13 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregator.cc
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/union-node.cc
M be/src/exprs/CMakeLists.txt
M be/src/exprs/agg-fn.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/case-expr.h
M be/src/exprs/compound-predicates.cc
M be/src/exprs/compound-predicates.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/literal.cc
M be/src/exprs/literal.h
M be/src/exprs/null-literal.cc
M be/src/exprs/null-literal.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/scalar-expr-ir.cc
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
A be/src/exprs/scalar-expr.inline.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
D be/src/exprs/slot-ref-ir.cc
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
R be/src/runtime/collection-value.cc
M be/src/runtime/collection-value.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tuple.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/util/tuple-row-compare.cc
M testdata/workloads/functional-query/queries/QueryTest/datastream-sender-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/disable-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
A testdata/workloads/tpcds-insert/queries/expr-insert.test
M tests/query_test/test_codegen.py
M tests/query_test/test_tpcds_queries.py
66 files changed, 899 insertions(+), 803 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12797/4
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12797
to look at the new patch set (#7).
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Based on initial draft patch by Pooja Nilangekar.
Codegen'd expressions can be executed in two ways - either by
being called directly from a fully codegend function, or from
interpreted code via a function pointer (previously
ScalarFnCall::scalar_fn_wrapper_).
This change moves the function pointer from ScalarFnCall to its
base class ScalarExpr, so the full expr tree can be codegen'd, not
just the ScalarFnCall subtrees. The key refactoring and improvements
are:
* ScalarExpr::Get*Val() switches between interpreted and the codegen'd
function pointer code paths in an inline function, avoiding a
virtual function call to ScalarFnCal::Get*Val().
* Boilerplate logic is moved to ScalarExpr::GetCodegendComputeFn(),
which calls a virtual function GetCodegenComputeFnImpl().
* ScalarFnCall's logic for deciding whether to interpret or codegen is
better abstracted and exposed to ScalarExpr as IsInterpretable()
and ShouldCodegen() methods.
* The ScalarExpr::codegend_compute_fn_ function pointer is only
populated for expressions that are "codegen entry points". These
include the roots of expr trees and non-root expressions
where the parent expression calls Get*Val() from the
pseudo-codegend GetCodegendComputeFnWrapper().
* ScalarFnCall is always initialised for interpreted execution.
Otherwise the function pointer is needed for non-root expressions,
e.g. to support ScalarExprEvaluator::GetConstantVal().
* Latent bugs/gaps for codegen of CollectionVal are fixed. CollectionVal
is modified to use the StringVal memory layout to allow code sharing
with StringVal. These fixes allowed simplification of
IsNotEmptyPredicate codegen (from IMPALA-7657).
IMPALA-7331 (CHAR codegen support functions) is also fixed because
it was simpler to enable CHAR codegen within ScalarExpr than to carry
forward the exiting CHAR workarounds from ScalarFnCall. The
CHAR-specific codegen support required in the scalar expr subsystem is
very limited. StringVal intermediates are used everywhere. Only
SlotRef actually operates on the different tuple layout, and the
required codegen support for SlotRef already exists for UDA
intermediates anyway.
Testing:
* Ran exhaustive tests.
Perf:
* Ran a basic insert benchmark, which went from 10.1s to 7.6s
create table foo stored as parquet as
select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end
from tpch30_parquet.lineitem;
* Ran a basic CHAR expr test:
set num_nodes=1;
set mt_dop=1;
select count(*) from lineitem
where cast(l_linestatus as CHAR(2)) = 'O ' and
cast(l_returnflag as CHAR(2)) = 'N '
The time spent in the scan went from 520ms to 220ms.
* Added perf regression test to tpcds-insert, similar to the manual
benchmark.
* Ran single-node TPC-H with large and small scale factors, to estimate
impact on execution perf and query startup time, respectively.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 6.84 | -0.18% | 4.49 | -0.31% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.58 | 2.47 | +4.18% | 1.29% | 0.88% | 5 | +4.12% | 2.31 | 5.81 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 4.81 | 4.61 | +4.33% | 2.18% | 2.15% | 5 | +3.91% | 1.73 | 3.09 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 26.45 | 26.16 | +1.09% | 0.37% | 0.50% | 5 | +1.36% | 2.02 | 3.94 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 15.92 | 15.75 | +1.09% | 2.87% | 1.65% | 5 | +0.88% | 0.29 | 0.73 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.38 | 2.35 | +1.12% | 1.64% | 1.11% | 5 | +0.80% | 1.15 | 1.26 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.13% | 7.68% | 5.37% | 5 | -0.34% | -0.29 | 0.27 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 18.10 | 18.02 | +0.42% | 2.70% | 0.56% | 5 | +0.28% | 0.29 | 0.34 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 4.72 | 4.72 | -0.04% | 1.20% | 1.65% | 5 | +0.05% | 0.00 | -0.04 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.92 | 3.93 | -0.26% | 1.08% | 2.36% | 5 | +0.20% | 0.58 | -0.23 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.27 | 1.27 | -0.28% | 0.22% | 0.88% | 5 | +0.09% | 0.29 | -0.68 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.64 | 2.65 | -0.45% | 1.65% | 0.65% | 5 | -0.24% | -0.58 | -0.57 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 3.10 | 3.13 | -0.76% | 1.47% | 1.12% | 5 | -0.21% | -0.29 | -0.93 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 1.20 | 1.21 | -0.80% | 2.26% | 2.47% | 5 | -0.82% | -1.15 | -0.53 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 1.97 | 1.99 | -1.37% | 1.84% | 3.21% | 5 | -0.47% | -0.58 | -0.83 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 11.53 | 11.63 | -0.91% | 0.46% | 0.49% | 5 | -0.95% | -2.02 | -3.08 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 5.13 | 5.21 | -1.51% | 2.24% | 4.05% | 5 | -0.94% | -0.58 | -0.73 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 3.61 | 3.66 | -1.40% | 0.66% | 0.79% | 5 | -1.33% | -1.73 | -3.05 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 19.42 | 19.71 | -1.52% | 1.34% | 1.39% | 5 | -1.22% | -1.44 | -1.76 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 5.08 | 5.15 | -1.49% | 1.34% | 0.73% | 5 | -1.35% | -1.44 | -2.20 |
| TPCH(30) | TPCH-Q15 | parquet / none / none | 3.42 | 3.49 | -1.92% | 0.93% | 1.47% | 5 | -1.53% | -1.15 | -2.49 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.15 | 1.19 | -3.17% | 2.27% | 1.95% | 5 | -4.21% | -1.15 | -2.41 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 9.26 | 9.63 | -3.85% | 0.62% | 0.59% | 5 | -3.78% | -2.31 | -10.25 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
Cluster Name: UNKNOWN
Lab Run Info: UNKNOWN
Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE ()
Baseline Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE (2019-03-19)
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(2) | parquet / none / none | 0.90 | -0.08% | 0.80 | -0.05% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(2) | TPCH-Q18 | parquet / none / none | 1.22 | 1.19 | +1.93% | 3.81% | 4.46% | 20 | +3.34% | 1.62 | 1.46 |
| TPCH(2) | TPCH-Q10 | parquet / none / none | 0.74 | 0.73 | +1.97% | 3.36% | 2.94% | 20 | +0.97% | 1.88 | 1.95 |
| TPCH(2) | TPCH-Q11 | parquet / none / none | 0.49 | 0.48 | +1.91% | 6.19% | 4.64% | 20 | +0.25% | 0.95 | 1.09 |
| TPCH(2) | TPCH-Q4 | parquet / none / none | 0.43 | 0.43 | +1.99% | 6.26% | 5.86% | 20 | +0.15% | 0.92 | 1.03 |
| TPCH(2) | TPCH-Q15 | parquet / none / none | 0.50 | 0.49 | +1.82% | 7.32% | 6.35% | 20 | +0.26% | 1.01 | 0.83 |
| TPCH(2) | TPCH-Q1 | parquet / none / none | 0.98 | 0.97 | +0.79% | 4.64% | 2.73% | 20 | +0.36% | 0.77 | 0.65 |
| TPCH(2) | TPCH-Q19 | parquet / none / none | 0.83 | 0.83 | +0.65% | 3.33% | 2.80% | 20 | +0.44% | 2.18 | 0.67 |
| TPCH(2) | TPCH-Q14 | parquet / none / none | 0.62 | 0.62 | +0.97% | 2.86% | 1.00% | 20 | +0.04% | 0.13 | 1.42 |
| TPCH(2) | TPCH-Q3 | parquet / none / none | 0.88 | 0.87 | +0.57% | 2.17% | 1.74% | 20 | +0.29% | 1.15 | 0.92 |
| TPCH(2) | TPCH-Q12 | parquet / none / none | 0.53 | 0.53 | +0.27% | 4.58% | 5.78% | 20 | +0.46% | 1.47 | 0.16 |
| TPCH(2) | TPCH-Q17 | parquet / none / none | 0.72 | 0.72 | +0.15% | 3.64% | 5.55% | 20 | +0.21% | 0.86 | 0.10 |
| TPCH(2) | TPCH-Q21 | parquet / none / none | 2.05 | 2.05 | +0.21% | 1.99% | 2.37% | 20 | +0.01% | 0.25 | 0.30 |
| TPCH(2) | TPCH-Q5 | parquet / none / none | 1.28 | 1.27 | +0.24% | 1.61% | 1.80% | 20 | -0.02% | -0.57 | 0.44 |
| TPCH(2) | TPCH-Q13 | parquet / none / none | 1.27 | 1.27 | -0.34% | 1.69% | 1.83% | 20 | -0.20% | -1.65 | -0.61 |
| TPCH(2) | TPCH-Q7 | parquet / none / none | 1.72 | 1.73 | -0.55% | 2.40% | 1.69% | 20 | -0.03% | -0.42 | -0.83 |
| TPCH(2) | TPCH-Q8 | parquet / none / none | 1.27 | 1.28 | -0.68% | 3.10% | 3.89% | 20 | -0.06% | -0.54 | -0.62 |
| TPCH(2) | TPCH-Q6 | parquet / none / none | 0.36 | 0.36 | -0.84% | 0.79% | 3.51% | 20 | -0.07% | -0.36 | -1.04 |
| TPCH(2) | TPCH-Q2 | parquet / none / none | 0.65 | 0.65 | -1.17% | 4.76% | 5.99% | 20 | -0.05% | -0.25 | -0.69 |
| TPCH(2) | TPCH-Q9 | parquet / none / none | 1.59 | 1.62 | -2.01% | 1.45% | 5.12% | 20 | -0.16% | -1.24 | -1.69 |
| TPCH(2) | TPCH-Q20 | parquet / none / none | 0.68 | 0.69 | -1.73% | 4.35% | 4.43% | 20 | -0.49% | -1.74 | -1.25 |
| TPCH(2) | TPCH-Q22 | parquet / none / none | 0.38 | 0.40 | -2.89% | 7.42% | 6.39% | 20 | -0.21% | -0.66 | -1.34 |
| TPCH(2) | TPCH-Q16 | parquet / none / none | 0.59 | 0.62 | -4.01% | 6.33% | 5.83% | 20 | -4.72% | -1.39 | -2.13 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregator.cc
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/union-node.cc
M be/src/exprs/CMakeLists.txt
M be/src/exprs/agg-fn.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/case-expr.h
M be/src/exprs/compound-predicates.cc
M be/src/exprs/compound-predicates.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/literal.cc
M be/src/exprs/literal.h
M be/src/exprs/null-literal.cc
M be/src/exprs/null-literal.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/scalar-expr-ir.cc
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
A be/src/exprs/scalar-expr.inline.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
D be/src/exprs/slot-ref-ir.cc
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
R be/src/runtime/collection-value.cc
M be/src/runtime/collection-value.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tuple.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/util/tuple-row-compare.cc
M testdata/workloads/functional-query/queries/QueryTest/datastream-sender-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/disable-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
A testdata/workloads/tpcds-insert/queries/expr-insert.test
M tests/query_test/test_codegen.py
M tests/query_test/test_tpcds_queries.py
66 files changed, 917 insertions(+), 865 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12797/7
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 17:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4265/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 17
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 May 2019 17:17:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 6:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG@41
PS6, Line 41: IMPALA-7331 (CHAR codegen support functions) is also fixed becaus
Shouldn't this speed up CHAR(N) manipulation? It would be nice to see some benchmarks for that too.
http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG@43
PS6, Line 43: The CHAR-specific
nit: wrap at 72 chars
http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG@53
PS6, Line 53: * Ran a basic insert benchmark, which went from 10.1s to 7.6s
: create table foo stored as parquet as
: select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end
: from tpch30_parquet.lineitem;
: * Added perf regression test to tpcds-insert, similar to the manual
: benchmark.
Is there a specific reason for using INSERT for benchmarking?
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/codegen/codegen-anyval.h
File be/src/codegen/codegen-anyval.h:
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/codegen/codegen-anyval.h@52
PS6, Line 52: /// TYPE_STRING,TYPE_VARCHAR,TYPE_CHAR,TYPE_FIXED_UDA_INTERMEDIATE/StringVal: { i64, i8* }
Collections could be also mentioned here.
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-expr.h
File be/src/exprs/scalar-expr.h:
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-expr.h@85
PS6, Line 85: One is implemented for each
: /// possible return type it supports (e.g. GetBooleanVal(), GetStringVal(), etc). The
: /// return type is a subclass of AnyVal (e.g. StringVal). One or more of these compute
: /// functions must be overridden by subclasses of ScalarExpr.
In the new version the *Interpreted() functions have to be overriden by subclasses.
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-expr.h@265
PS6, Line 265: Interpretd
typo
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.h
File be/src/exprs/scalar-fn-call.h:
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.h@47
PS6, Line 47: GetCodegendComputeFn
"Impl" is missing.
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.h@48
PS6, Line 48: e
nit: long line could be fixed if this comment is touched
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.h@49
PS6, Line 49: If codegen is enabled, ScalarFnCall's Get*Val() compute
: /// functions are wrappers around this codegen'd function.
This sentence would be more relevant in scalar-expr.h
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc@483
PS6, Line 483: // TODO: macroify this?
+1 for this TODO, I think that it would be better to do this with macros both in declaration and implementation, and have a single macro that calls it for all *Val that supports codegen. This would make it easier to avoid mistakes when adding a new type, e.g. Date.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 05 Apr 2019 16:30:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2481/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Mar 2019 16:44:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2765/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 9
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Apr 2019 22:27:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 10:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.h
File be/src/exprs/scalar-expr.h:
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.h@87
PS10, Line 87: Get*Val() dispatches to either
: /// a codegen'd function pointer or to an interpreted implementation Get*ValInterpreted()
: /// These interpreted functions must be overridden by subclasses of ScalarExpr for every
: /// type that they may return.
: ///
: /// The two main usage patterns for ScalarExpr are:
: /// * The codegen'd expressions are called from other codegen'd functions, e.g. from a
: /// codegen'd join implementation
: /// * Get*Val() is called on the root of each expression subtree by interpreted code.
: /// We can optimize for the second usage pattern by filling in the codegen'd function
: /// pointer (codegend_compute_fn_) in root of each ScalarExpr tree. Individual callsites
: /// can disable this optimisation if it's not needed. Expr subtrees can be evaluated
: /// (e.g. by ScalarExprEvaluator::GetConstValue()) but may fail back to a slower
: /// interpreted implementation.
> These sets of comments seem to fit better after line 107-114. Reading this
Done
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.h@170
PS10, Line 170: bool is_codegen_entry_point,
> I wonder how much measurable benefit we get from this. IMHO, this can be an
I thought about this because it would have made my life easier - that change was the harder part of the patch. I added some explanation to the commit for my decision to couple the changes. I wasn't comfortable separating them because, at minimum, adding all those extra codegen'd function pointers will regress codegen time a bit, and I think it will regress it significantly for some exprs because of the blow-up in number of entry points. It might also make the code generated worth because of changes in inlining decisions. E.g. before this change a codegen'd ConstantExpr would be an internal function with only have a single IR caller, so will be aggressively inlined. But afterwards it's external so may not be inlined.
E.g. if you have "x = 1", then previously there was one function pointer populated because there's only one ScalarFnCall node. But if we add support for function pointers in every node without being selective about it, then we would have 3 entry points.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.h@288
PS10, Line 288: If 'is_entry_point' is true, this indicates that Get*Val()
: /// may be called directly from interpreted code and that we should generate an entry
: /// point into the codegen'd code.
> Please see comments in scalar-expr.cc. 'is_entry_point' may not need to be
Ack
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.h@349
PS10, Line 349: /// The vast majority of exprs support interpretation, so default to true.
> May want to point out that expressions which aren't interpretable should ov
I think I can make these protected so that the concepts are internal to this class hierarchy and there isn't any additional surface area. I think it doesn't add complexity within the ScalarExpr subsystem since the concepts were already present implicitly.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.h@375
PS10, Line 375: codegend_compute_fn_ = nullptr;
> Please document that this is left as null if this scalar expression is not
I added some explanation of when it is populated to avoid the double negative.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.cc
File be/src/exprs/scalar-expr.cc:
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.cc@284
PS10, Line 284: bool is_entry_point
> May be I didn't follow all the details correctly but it seems to me that all these plumbing for passing this flag around is only used in GetCodegendComputeFn().
Yeah that's ultimately what happens, the challenge is that GetCodegendComputeFn() can be called multiple times from different places, and the callers have the information about whether it should be an entry point.
> In theory, only the top level expression (i.e. the root of the scalar expression tree) may require exposing itself with a function pointer after codegen (i.e. it's an entry point).
This assumption is false - GetCodegendComputeFnWrapper() generates a codegen'd function that calls the interpreted path of its child. We need to generate entry points there to avoid a perf cliff where the whole subtree doesn't get codegen'd (which would be a regression from the old behaviour too).
> More importantly, this seems to be implementation specific details / optimization that we probably should try not to expose it in the interface of Init() if possible.
ScalarExpr doesn't know the context in which it is used. E.g. if it's the child of an AggregateExpr, then it's not an entry point but doesn't know that based on local information. There are actually a bunch more places where the expr is always called from codegen'd code and we could save some overhead, but I didn't go through and find them all.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.cc@409
PS10, Line 409: llvm::Function* static_getval_fn = GetStaticGetValWrapper(type(), codegen);
> if (static_getval_fn == nullptr) {
I think this would be a case of writing extra code to handle an invariant violation, which we don't want to do because of complication and the inherent impossibility of testing. I don't think there's a real risk of this failing at runtime since it's only looking up a function from the builtins.
I modified the comment of GetStaticGetValWrapper() to be unambiguous about the invariant.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.inline.h
File be/src/exprs/scalar-expr.inline.h:
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.inline.h@25
PS10, Line 25: #define SCALAR_EXPR_GET_VAL(val_type, type_validation)
> Please document the purpose of this macro and its arguments. Also please al
Done. I think the behaviour is better documented on the declaration in scalar-expr.h, not the implementation. I think the comment there already covers that point.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.inline.h@27
PS10, Line 27: ScalarExpr::Get##val_type(
> I am not a big fan of this type of macro code generation as it makes findin
Point taken. I do think it's nice to avoid the semi-subtle boilerplate so that we don't accidentally forgot to update one place.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-fn-call.h
File be/src/exprs/scalar-fn-call.h:
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-fn-call.h@87
PS10, Line 87: virtual bool IsInterpretable() const override;
> Warrants a comment on why this is overridden here.
Done
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-fn-call.h@112
PS10, Line 112: /// functions. Not set for IR UDFs.
> Please consider keeping the original part of the comment about that this is
Done
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-fn-call.cc@484
PS10, Line 484: #define GET_VAL_INTERPRETED(val_type, type_validation) \
> Similar comments as scalar-expr.inline.h: May help to add a comment to say
Done
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 10
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 23 Apr 2019 18:43:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 7: Code-Review+1
(3 comments)
I am ok with the patch in general, but I do not have enough codegen experience to give +2. If no other people wants to jump on it, I can dig deeper but it may take some time.
http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG@41
PS6, Line 41: IMPALA-7331 (CHAR codegen support functions) is also fixed becaus
> I added a toy manual benchmark below that shows a significant improvement.
I am not sure - I do not know how many people use CHAR(N). My intuition says that CHAR(N) makes sense for columns with short strings but large NDV, but if its usage is discouraged in Impala, then I guess that we shouldn't put effort in benchmarking. I wouldn't add it to targeted-perf in this patch, but creating a Jira for it never hurts. :)
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc@483
PS6, Line 483: #pragma push_macro("GET
> Done
I would prefer to use a macro in the header too - in such a way that gets the list of "*Val"s from a macro, to avoid the possibility of forgetting to override it for a new type. I am ok with not doing it in this change.
The simplest way would be probably to use a macro like GENERATE_GET_VAL_INTERPRETED_OVERRIDES_FOR_ALL_SCALAR_TYPES, and use it in the few classes that implement it for all scalar types. A few classes override it for CollectionVal too, so it could be added separately, or a macro could be created that includes CollectionVal too.
http://gerrit.cloudera.org:8080/#/c/12797/7/be/src/udf/udf-internal.h
File be/src/udf/udf-internal.h:
http://gerrit.cloudera.org:8080/#/c/12797/7/be/src/udf/udf-internal.h@299
PS7, Line 299: // Matches memory layout of StringVal to allow sharing of support in CodegenAnyval.
: // Also is denser this can fit into 16 bytes along with the initial is_null member.
This could be enforced with static_assert()s:
static_assert(sizeof(CollectionVal) == sizeof(StringVal), "Wrong size.");
static_assert(offsetof(CollectionVal, num_tuples) == offsetof(StringVal, len), "Wrong offset.");
static_assert(offsetof(CollectionVal, ptr) == offsetof(StringVal, ptr), "Wrong offset.");
offsetof works in GCC, I didn't try it in Clang.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Apr 2019 15:47:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 16:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4257/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 16
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2019 21:52:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 17: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 17
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 May 2019 17:17:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2480/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Mar 2019 16:40:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 16: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/4256/
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 16
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2019 21:53:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12797
to look at the new patch set (#11).
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Based on initial draft patch by Pooja Nilangekar.
Codegen'd expressions can be executed in two ways - either by
being called directly from a fully codegend function, or from
interpreted code via a function pointer (previously
ScalarFnCall::scalar_fn_wrapper_).
This change moves the function pointer from ScalarFnCall to its
base class ScalarExpr, so the full expr tree can be codegen'd, not
just the ScalarFnCall subtrees. The key refactoring and improvements
are:
* ScalarExpr::Get*Val() switches between interpreted and the codegen'd
function pointer code paths in an inline function, avoiding a
virtual function call to ScalarFnCal::Get*Val().
* Boilerplate logic is moved to ScalarExpr::GetCodegendComputeFn(),
which calls a virtual function GetCodegenComputeFnImpl().
* ScalarFnCall's logic for deciding whether to interpret or codegen is
better abstracted and exposed to ScalarExpr as IsInterpretable()
and ShouldCodegen() methods.
* The ScalarExpr::codegend_compute_fn_ function pointer is only
populated for expressions that are "codegen entry points". These
include the roots of expr trees and non-root expressions
where the parent expression calls Get*Val() from the
pseudo-codegend GetCodegendComputeFnWrapper().
* ScalarFnCall is always initialised for interpreted execution.
Otherwise the function pointer is needed for non-root expressions,
e.g. to support ScalarExprEvaluator::GetConstantVal().
* Latent bugs/gaps for codegen of CollectionVal are fixed. CollectionVal
is modified to use the StringVal memory layout to allow code sharing
with StringVal. These fixes allowed simplification of
IsNotEmptyPredicate codegen (from IMPALA-7657).
I chose to tackle two problems in one change - adding support for
generating codegen'd function pointers for all ScalarExprs, and adding
the "entry point" concept - to avoid a blow-up in the number of
codegen'd entry points that could lead to longer codegen times and/or
worse code because of inlining changes.
IMPALA-7331 (CHAR codegen support functions) is also fixed because
it was simpler to enable CHAR codegen within ScalarExpr than to carry
forward the exiting CHAR workarounds from ScalarFnCall. The
CHAR-specific codegen support required in the scalar expr subsystem is
very limited. StringVal intermediates are used everywhere. Only
SlotRef actually operates on the different tuple layout, and the
required codegen support for SlotRef already exists for UDA
intermediates anyway.
Testing:
* Ran exhaustive tests.
Perf:
* Ran a basic insert benchmark, which went from 10.1s to 7.6s
create table foo stored as parquet as
select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end
from tpch30_parquet.lineitem;
* Ran a basic CHAR expr test:
set num_nodes=1;
set mt_dop=1;
select count(*) from lineitem
where cast(l_linestatus as CHAR(2)) = 'O ' and
cast(l_returnflag as CHAR(2)) = 'N '
The time spent in the scan went from 520ms to 220ms.
* Added perf regression test to tpcds-insert, similar to the manual
benchmark.
* Ran single-node TPC-H with large and small scale factors, to estimate
impact on execution perf and query startup time, respectively.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 6.84 | -0.18% | 4.49 | -0.31% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.58 | 2.47 | +4.18% | 1.29% | 0.88% | 5 | +4.12% | 2.31 | 5.81 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 4.81 | 4.61 | +4.33% | 2.18% | 2.15% | 5 | +3.91% | 1.73 | 3.09 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 26.45 | 26.16 | +1.09% | 0.37% | 0.50% | 5 | +1.36% | 2.02 | 3.94 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 15.92 | 15.75 | +1.09% | 2.87% | 1.65% | 5 | +0.88% | 0.29 | 0.73 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.38 | 2.35 | +1.12% | 1.64% | 1.11% | 5 | +0.80% | 1.15 | 1.26 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.13% | 7.68% | 5.37% | 5 | -0.34% | -0.29 | 0.27 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 18.10 | 18.02 | +0.42% | 2.70% | 0.56% | 5 | +0.28% | 0.29 | 0.34 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 4.72 | 4.72 | -0.04% | 1.20% | 1.65% | 5 | +0.05% | 0.00 | -0.04 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.92 | 3.93 | -0.26% | 1.08% | 2.36% | 5 | +0.20% | 0.58 | -0.23 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.27 | 1.27 | -0.28% | 0.22% | 0.88% | 5 | +0.09% | 0.29 | -0.68 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.64 | 2.65 | -0.45% | 1.65% | 0.65% | 5 | -0.24% | -0.58 | -0.57 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 3.10 | 3.13 | -0.76% | 1.47% | 1.12% | 5 | -0.21% | -0.29 | -0.93 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 1.20 | 1.21 | -0.80% | 2.26% | 2.47% | 5 | -0.82% | -1.15 | -0.53 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 1.97 | 1.99 | -1.37% | 1.84% | 3.21% | 5 | -0.47% | -0.58 | -0.83 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 11.53 | 11.63 | -0.91% | 0.46% | 0.49% | 5 | -0.95% | -2.02 | -3.08 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 5.13 | 5.21 | -1.51% | 2.24% | 4.05% | 5 | -0.94% | -0.58 | -0.73 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 3.61 | 3.66 | -1.40% | 0.66% | 0.79% | 5 | -1.33% | -1.73 | -3.05 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 19.42 | 19.71 | -1.52% | 1.34% | 1.39% | 5 | -1.22% | -1.44 | -1.76 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 5.08 | 5.15 | -1.49% | 1.34% | 0.73% | 5 | -1.35% | -1.44 | -2.20 |
| TPCH(30) | TPCH-Q15 | parquet / none / none | 3.42 | 3.49 | -1.92% | 0.93% | 1.47% | 5 | -1.53% | -1.15 | -2.49 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.15 | 1.19 | -3.17% | 2.27% | 1.95% | 5 | -4.21% | -1.15 | -2.41 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 9.26 | 9.63 | -3.85% | 0.62% | 0.59% | 5 | -3.78% | -2.31 | -10.25 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
Cluster Name: UNKNOWN
Lab Run Info: UNKNOWN
Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE ()
Baseline Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE (2019-03-19)
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(2) | parquet / none / none | 0.90 | -0.08% | 0.80 | -0.05% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(2) | TPCH-Q18 | parquet / none / none | 1.22 | 1.19 | +1.93% | 3.81% | 4.46% | 20 | +3.34% | 1.62 | 1.46 |
| TPCH(2) | TPCH-Q10 | parquet / none / none | 0.74 | 0.73 | +1.97% | 3.36% | 2.94% | 20 | +0.97% | 1.88 | 1.95 |
| TPCH(2) | TPCH-Q11 | parquet / none / none | 0.49 | 0.48 | +1.91% | 6.19% | 4.64% | 20 | +0.25% | 0.95 | 1.09 |
| TPCH(2) | TPCH-Q4 | parquet / none / none | 0.43 | 0.43 | +1.99% | 6.26% | 5.86% | 20 | +0.15% | 0.92 | 1.03 |
| TPCH(2) | TPCH-Q15 | parquet / none / none | 0.50 | 0.49 | +1.82% | 7.32% | 6.35% | 20 | +0.26% | 1.01 | 0.83 |
| TPCH(2) | TPCH-Q1 | parquet / none / none | 0.98 | 0.97 | +0.79% | 4.64% | 2.73% | 20 | +0.36% | 0.77 | 0.65 |
| TPCH(2) | TPCH-Q19 | parquet / none / none | 0.83 | 0.83 | +0.65% | 3.33% | 2.80% | 20 | +0.44% | 2.18 | 0.67 |
| TPCH(2) | TPCH-Q14 | parquet / none / none | 0.62 | 0.62 | +0.97% | 2.86% | 1.00% | 20 | +0.04% | 0.13 | 1.42 |
| TPCH(2) | TPCH-Q3 | parquet / none / none | 0.88 | 0.87 | +0.57% | 2.17% | 1.74% | 20 | +0.29% | 1.15 | 0.92 |
| TPCH(2) | TPCH-Q12 | parquet / none / none | 0.53 | 0.53 | +0.27% | 4.58% | 5.78% | 20 | +0.46% | 1.47 | 0.16 |
| TPCH(2) | TPCH-Q17 | parquet / none / none | 0.72 | 0.72 | +0.15% | 3.64% | 5.55% | 20 | +0.21% | 0.86 | 0.10 |
| TPCH(2) | TPCH-Q21 | parquet / none / none | 2.05 | 2.05 | +0.21% | 1.99% | 2.37% | 20 | +0.01% | 0.25 | 0.30 |
| TPCH(2) | TPCH-Q5 | parquet / none / none | 1.28 | 1.27 | +0.24% | 1.61% | 1.80% | 20 | -0.02% | -0.57 | 0.44 |
| TPCH(2) | TPCH-Q13 | parquet / none / none | 1.27 | 1.27 | -0.34% | 1.69% | 1.83% | 20 | -0.20% | -1.65 | -0.61 |
| TPCH(2) | TPCH-Q7 | parquet / none / none | 1.72 | 1.73 | -0.55% | 2.40% | 1.69% | 20 | -0.03% | -0.42 | -0.83 |
| TPCH(2) | TPCH-Q8 | parquet / none / none | 1.27 | 1.28 | -0.68% | 3.10% | 3.89% | 20 | -0.06% | -0.54 | -0.62 |
| TPCH(2) | TPCH-Q6 | parquet / none / none | 0.36 | 0.36 | -0.84% | 0.79% | 3.51% | 20 | -0.07% | -0.36 | -1.04 |
| TPCH(2) | TPCH-Q2 | parquet / none / none | 0.65 | 0.65 | -1.17% | 4.76% | 5.99% | 20 | -0.05% | -0.25 | -0.69 |
| TPCH(2) | TPCH-Q9 | parquet / none / none | 1.59 | 1.62 | -2.01% | 1.45% | 5.12% | 20 | -0.16% | -1.24 | -1.69 |
| TPCH(2) | TPCH-Q20 | parquet / none / none | 0.68 | 0.69 | -1.73% | 4.35% | 4.43% | 20 | -0.49% | -1.74 | -1.25 |
| TPCH(2) | TPCH-Q22 | parquet / none / none | 0.38 | 0.40 | -2.89% | 7.42% | 6.39% | 20 | -0.21% | -0.66 | -1.34 |
| TPCH(2) | TPCH-Q16 | parquet / none / none | 0.59 | 0.62 | -4.01% | 6.33% | 5.83% | 20 | -4.72% | -1.39 | -2.13 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregator.cc
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/union-node.cc
M be/src/exprs/CMakeLists.txt
M be/src/exprs/agg-fn.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/case-expr.h
M be/src/exprs/compound-predicates.cc
M be/src/exprs/compound-predicates.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/literal.cc
M be/src/exprs/literal.h
M be/src/exprs/null-literal.cc
M be/src/exprs/null-literal.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/scalar-expr-ir.cc
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
A be/src/exprs/scalar-expr.inline.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
D be/src/exprs/slot-ref-ir.cc
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
R be/src/runtime/collection-value.cc
M be/src/runtime/collection-value.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tuple.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/util/tuple-row-compare.cc
M testdata/workloads/functional-query/queries/QueryTest/datastream-sender-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/disable-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
A testdata/workloads/tpcds-insert/queries/expr-insert.test
M tests/query_test/test_codegen.py
M tests/query_test/test_tpcds_queries.py
66 files changed, 820 insertions(+), 890 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12797/11
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 11
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Based on initial draft patch by Pooja Nilangekar.
Codegen'd expressions can be executed in two ways - either by
being called directly from a fully codegend function, or from
interpreted code via a function pointer (previously
ScalarFnCall::scalar_fn_wrapper_).
This change moves the function pointer from ScalarFnCall to its
base class ScalarExpr, so the full expr tree can be codegen'd, not
just the ScalarFnCall subtrees. The key refactoring and improvements
are:
* ScalarExpr::Get*Val() switches between interpreted and the codegen'd
function pointer code paths in an inline function, avoiding a
virtual function call to ScalarFnCal::Get*Val().
* Boilerplate logic is moved to ScalarExpr::GetCodegendComputeFn(),
which calls a virtual function GetCodegenComputeFnImpl().
* ScalarFnCall's logic for deciding whether to interpret or codegen is
better abstracted and exposed to ScalarExpr as IsInterpretable()
and ShouldCodegen() methods.
* The ScalarExpr::codegend_compute_fn_ function pointer is only
populated for expressions that are "codegen entry points". These
include the roots of expr trees and non-root expressions
where the parent expression calls Get*Val() from the
pseudo-codegend GetCodegendComputeFnWrapper().
* ScalarFnCall is always initialised for interpreted execution.
Otherwise the function pointer is needed for non-root expressions,
e.g. to support ScalarExprEvaluator::GetConstantVal().
* Latent bugs/gaps for codegen of CollectionVal are fixed. CollectionVal
is modified to use the StringVal memory layout to allow code sharing
with StringVal. These fixes allowed simplification of
IsNotEmptyPredicate codegen (from IMPALA-7657).
I chose to tackle two problems in one change - adding support for
generating codegen'd function pointers for all ScalarExprs, and adding
the "entry point" concept - to avoid a blow-up in the number of
codegen'd entry points that could lead to longer codegen times and/or
worse code because of inlining changes.
IMPALA-7331 (CHAR codegen support functions) is also fixed because
it was simpler to enable CHAR codegen within ScalarExpr than to carry
forward the exiting CHAR workarounds from ScalarFnCall. The
CHAR-specific codegen support required in the scalar expr subsystem is
very limited. StringVal intermediates are used everywhere. Only
SlotRef actually operates on the different tuple layout, and the
required codegen support for SlotRef already exists for UDA
intermediates anyway.
Testing:
* Ran exhaustive tests.
Perf:
* Ran a basic insert benchmark, which went from 10.1s to 7.6s
create table foo stored as parquet as
select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end
from tpch30_parquet.lineitem;
* Ran a basic CHAR expr test:
set num_nodes=1;
set mt_dop=1;
select count(*) from lineitem
where cast(l_linestatus as CHAR(2)) = 'O ' and
cast(l_returnflag as CHAR(2)) = 'N '
The time spent in the scan went from 520ms to 220ms.
* Added perf regression test to tpcds-insert, similar to the manual
benchmark.
* Ran single-node TPC-H with large and small scale factors, to estimate
impact on execution perf and query startup time, respectively.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 6.84 | -0.18% | 4.49 | -0.31% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.58 | 2.47 | +4.18% | 1.29% | 0.88% | 5 | +4.12% | 2.31 | 5.81 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 4.81 | 4.61 | +4.33% | 2.18% | 2.15% | 5 | +3.91% | 1.73 | 3.09 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 26.45 | 26.16 | +1.09% | 0.37% | 0.50% | 5 | +1.36% | 2.02 | 3.94 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 15.92 | 15.75 | +1.09% | 2.87% | 1.65% | 5 | +0.88% | 0.29 | 0.73 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.38 | 2.35 | +1.12% | 1.64% | 1.11% | 5 | +0.80% | 1.15 | 1.26 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.13% | 7.68% | 5.37% | 5 | -0.34% | -0.29 | 0.27 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 18.10 | 18.02 | +0.42% | 2.70% | 0.56% | 5 | +0.28% | 0.29 | 0.34 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 4.72 | 4.72 | -0.04% | 1.20% | 1.65% | 5 | +0.05% | 0.00 | -0.04 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.92 | 3.93 | -0.26% | 1.08% | 2.36% | 5 | +0.20% | 0.58 | -0.23 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.27 | 1.27 | -0.28% | 0.22% | 0.88% | 5 | +0.09% | 0.29 | -0.68 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.64 | 2.65 | -0.45% | 1.65% | 0.65% | 5 | -0.24% | -0.58 | -0.57 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 3.10 | 3.13 | -0.76% | 1.47% | 1.12% | 5 | -0.21% | -0.29 | -0.93 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 1.20 | 1.21 | -0.80% | 2.26% | 2.47% | 5 | -0.82% | -1.15 | -0.53 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 1.97 | 1.99 | -1.37% | 1.84% | 3.21% | 5 | -0.47% | -0.58 | -0.83 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 11.53 | 11.63 | -0.91% | 0.46% | 0.49% | 5 | -0.95% | -2.02 | -3.08 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 5.13 | 5.21 | -1.51% | 2.24% | 4.05% | 5 | -0.94% | -0.58 | -0.73 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 3.61 | 3.66 | -1.40% | 0.66% | 0.79% | 5 | -1.33% | -1.73 | -3.05 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 19.42 | 19.71 | -1.52% | 1.34% | 1.39% | 5 | -1.22% | -1.44 | -1.76 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 5.08 | 5.15 | -1.49% | 1.34% | 0.73% | 5 | -1.35% | -1.44 | -2.20 |
| TPCH(30) | TPCH-Q15 | parquet / none / none | 3.42 | 3.49 | -1.92% | 0.93% | 1.47% | 5 | -1.53% | -1.15 | -2.49 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.15 | 1.19 | -3.17% | 2.27% | 1.95% | 5 | -4.21% | -1.15 | -2.41 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 9.26 | 9.63 | -3.85% | 0.62% | 0.59% | 5 | -3.78% | -2.31 | -10.25 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
Cluster Name: UNKNOWN
Lab Run Info: UNKNOWN
Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE ()
Baseline Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE (2019-03-19)
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(2) | parquet / none / none | 0.90 | -0.08% | 0.80 | -0.05% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(2) | TPCH-Q18 | parquet / none / none | 1.22 | 1.19 | +1.93% | 3.81% | 4.46% | 20 | +3.34% | 1.62 | 1.46 |
| TPCH(2) | TPCH-Q10 | parquet / none / none | 0.74 | 0.73 | +1.97% | 3.36% | 2.94% | 20 | +0.97% | 1.88 | 1.95 |
| TPCH(2) | TPCH-Q11 | parquet / none / none | 0.49 | 0.48 | +1.91% | 6.19% | 4.64% | 20 | +0.25% | 0.95 | 1.09 |
| TPCH(2) | TPCH-Q4 | parquet / none / none | 0.43 | 0.43 | +1.99% | 6.26% | 5.86% | 20 | +0.15% | 0.92 | 1.03 |
| TPCH(2) | TPCH-Q15 | parquet / none / none | 0.50 | 0.49 | +1.82% | 7.32% | 6.35% | 20 | +0.26% | 1.01 | 0.83 |
| TPCH(2) | TPCH-Q1 | parquet / none / none | 0.98 | 0.97 | +0.79% | 4.64% | 2.73% | 20 | +0.36% | 0.77 | 0.65 |
| TPCH(2) | TPCH-Q19 | parquet / none / none | 0.83 | 0.83 | +0.65% | 3.33% | 2.80% | 20 | +0.44% | 2.18 | 0.67 |
| TPCH(2) | TPCH-Q14 | parquet / none / none | 0.62 | 0.62 | +0.97% | 2.86% | 1.00% | 20 | +0.04% | 0.13 | 1.42 |
| TPCH(2) | TPCH-Q3 | parquet / none / none | 0.88 | 0.87 | +0.57% | 2.17% | 1.74% | 20 | +0.29% | 1.15 | 0.92 |
| TPCH(2) | TPCH-Q12 | parquet / none / none | 0.53 | 0.53 | +0.27% | 4.58% | 5.78% | 20 | +0.46% | 1.47 | 0.16 |
| TPCH(2) | TPCH-Q17 | parquet / none / none | 0.72 | 0.72 | +0.15% | 3.64% | 5.55% | 20 | +0.21% | 0.86 | 0.10 |
| TPCH(2) | TPCH-Q21 | parquet / none / none | 2.05 | 2.05 | +0.21% | 1.99% | 2.37% | 20 | +0.01% | 0.25 | 0.30 |
| TPCH(2) | TPCH-Q5 | parquet / none / none | 1.28 | 1.27 | +0.24% | 1.61% | 1.80% | 20 | -0.02% | -0.57 | 0.44 |
| TPCH(2) | TPCH-Q13 | parquet / none / none | 1.27 | 1.27 | -0.34% | 1.69% | 1.83% | 20 | -0.20% | -1.65 | -0.61 |
| TPCH(2) | TPCH-Q7 | parquet / none / none | 1.72 | 1.73 | -0.55% | 2.40% | 1.69% | 20 | -0.03% | -0.42 | -0.83 |
| TPCH(2) | TPCH-Q8 | parquet / none / none | 1.27 | 1.28 | -0.68% | 3.10% | 3.89% | 20 | -0.06% | -0.54 | -0.62 |
| TPCH(2) | TPCH-Q6 | parquet / none / none | 0.36 | 0.36 | -0.84% | 0.79% | 3.51% | 20 | -0.07% | -0.36 | -1.04 |
| TPCH(2) | TPCH-Q2 | parquet / none / none | 0.65 | 0.65 | -1.17% | 4.76% | 5.99% | 20 | -0.05% | -0.25 | -0.69 |
| TPCH(2) | TPCH-Q9 | parquet / none / none | 1.59 | 1.62 | -2.01% | 1.45% | 5.12% | 20 | -0.16% | -1.24 | -1.69 |
| TPCH(2) | TPCH-Q20 | parquet / none / none | 0.68 | 0.69 | -1.73% | 4.35% | 4.43% | 20 | -0.49% | -1.74 | -1.25 |
| TPCH(2) | TPCH-Q22 | parquet / none / none | 0.38 | 0.40 | -2.89% | 7.42% | 6.39% | 20 | -0.21% | -0.66 | -1.34 |
| TPCH(2) | TPCH-Q16 | parquet / none / none | 0.59 | 0.62 | -4.01% | 6.33% | 5.83% | 20 | -4.72% | -1.39 | -2.13 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Reviewed-on: http://gerrit.cloudera.org:8080/12797
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregator.cc
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/union-node.cc
M be/src/exprs/CMakeLists.txt
M be/src/exprs/agg-fn.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/case-expr.h
M be/src/exprs/compound-predicates.cc
M be/src/exprs/compound-predicates.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/literal.cc
M be/src/exprs/literal.h
M be/src/exprs/null-literal.cc
M be/src/exprs/null-literal.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/scalar-expr-ir.cc
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
A be/src/exprs/scalar-expr.inline.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
D be/src/exprs/slot-ref-ir.cc
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
R be/src/runtime/collection-value.cc
M be/src/runtime/collection-value.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tuple.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/util/tuple-row-compare.cc
M testdata/workloads/functional-query/queries/QueryTest/datastream-sender-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/disable-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
A testdata/workloads/tpcds-insert/queries/expr-insert.test
M tests/query_test/test_codegen.py
M tests/query_test/test_tpcds_queries.py
66 files changed, 841 insertions(+), 919 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 18
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 16:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4256/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 16
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2019 21:48:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 10: Code-Review+1
(1 comment)
I've done a decent amount of codegen work, so I'm comfortable +2ing this if Michael wants (I know he's got a lot of reviews on his plate at the moment), but I'll give it a +1 for now in case he wants to take a look
http://gerrit.cloudera.org:8080/#/c/12797/8/tests/query_test/test_tpcds_queries.py
File tests/query_test/test_tpcds_queries.py:
http://gerrit.cloudera.org:8080/#/c/12797/8/tests/query_test/test_tpcds_queries.py@528
PS8, Line 528: def test_expr_insert(self, vector):
> Yeah this is true, test_partitioned_insert is actually broken. I did run th
That's fine with me, this patch is already quite large
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 10
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Apr 2019 21:47:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 14:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3226/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 14
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2019 22:23:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3927/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Mar 2019 15:57:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 10:
I'm comfortable with the two sets of eyes on it, but will leave it to michael's judgement if we wants to take a look.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 10
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Apr 2019 21:51:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 12:
(1 comment)
Answered the one question. Will address other comments later.
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-fn-call.cc@143
PS12, Line 143: // We're in the interpreted path (i.e. no JIT). Populate our FunctionContext's
: // staging_input_vals, which will be reused across calls to scalar_fn_ on the
: // interpreted code path..
: vector<AnyVal*>* input_vals = fn_ctx->impl()->staging_input_vals();
: for (int i = 0; i < NumFixedArgs(); ++i) {
: AnyVal* input_val;
: RETURN_IF_ERROR(AllocateAnyVal(state, eval->expr_perm_pool(), children_[i]->type(),
: "Could not allocate expression value", &input_val));
: input_vals->push_back(input_val);
: }
> Not sure I understand this part. Does it mean that we will do this initiali
Yes, the problem I ran into is that code like the below code to GetConstValue() requires that Get*Value() potentially work on all subtrees. This worked before because we filled in function pointers in all subtrees, but broke when we changed that.
We could try to be clever and only initialise some subtrees for interpreted execution, but I eventually decided that was an unnecessary microoptimisation and added complexity in trying to reason about which subtrees it was valid to call Get*Value() on.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 23:19:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 6:
(3 comments)
Michael is the obvious person to +2 since he did a bunch of work in codegen and the expr framework. Not sure if he has time to look.
http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG@41
PS6, Line 41: IMPALA-7331 (CHAR codegen support functions) is also fixed becaus
> I am not sure - I do not know how many people use CHAR(N). My intuition say
IMPALA-8402
I have seen people use it in production, often as a result from porting over existing schemas from other DBs or using patterns from other DBs.
The fixed-width layout does have advantages for short strings (although that's nullified by missing codegen), but the semantics of automatically adding and removing spaces on the end are a little weird and you can't separate the semantics and the memory layout.
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc@483
PS6, Line 483: // TODO: macroify this?
> I would prefer to use a macro in the header too - in such a way that gets t
I like this idea. Went ahead with it as you suggested. It was enough to turn this into a net-negative patch in line count (although that's not overly meaningful)
http://gerrit.cloudera.org:8080/#/c/12797/7/be/src/udf/udf-internal.h
File be/src/udf/udf-internal.h:
http://gerrit.cloudera.org:8080/#/c/12797/7/be/src/udf/udf-internal.h@299
PS7, Line 299: // Matches memory layout of StringVal to allow sharing of support in CodegenAnyval.
: // Also is denser this can fit into 16 bytes along with the initial is_null member.
> This could be enforced with static_assert()s:
This is a great idea. Wish I'd thought of it. I assume clang supports it. I will lazily test this assumption with the precommit clang-tidy job :).
I had to disable a warning for GCC since it is a stickler about disallowing offsetof for any class with a constructor.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Apr 2019 22:24:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 17: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 17
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 15 May 2019 22:34:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12797
to look at the new patch set (#13).
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Based on initial draft patch by Pooja Nilangekar.
Codegen'd expressions can be executed in two ways - either by
being called directly from a fully codegend function, or from
interpreted code via a function pointer (previously
ScalarFnCall::scalar_fn_wrapper_).
This change moves the function pointer from ScalarFnCall to its
base class ScalarExpr, so the full expr tree can be codegen'd, not
just the ScalarFnCall subtrees. The key refactoring and improvements
are:
* ScalarExpr::Get*Val() switches between interpreted and the codegen'd
function pointer code paths in an inline function, avoiding a
virtual function call to ScalarFnCal::Get*Val().
* Boilerplate logic is moved to ScalarExpr::GetCodegendComputeFn(),
which calls a virtual function GetCodegenComputeFnImpl().
* ScalarFnCall's logic for deciding whether to interpret or codegen is
better abstracted and exposed to ScalarExpr as IsInterpretable()
and ShouldCodegen() methods.
* The ScalarExpr::codegend_compute_fn_ function pointer is only
populated for expressions that are "codegen entry points". These
include the roots of expr trees and non-root expressions
where the parent expression calls Get*Val() from the
pseudo-codegend GetCodegendComputeFnWrapper().
* ScalarFnCall is always initialised for interpreted execution.
Otherwise the function pointer is needed for non-root expressions,
e.g. to support ScalarExprEvaluator::GetConstantVal().
* Latent bugs/gaps for codegen of CollectionVal are fixed. CollectionVal
is modified to use the StringVal memory layout to allow code sharing
with StringVal. These fixes allowed simplification of
IsNotEmptyPredicate codegen (from IMPALA-7657).
I chose to tackle two problems in one change - adding support for
generating codegen'd function pointers for all ScalarExprs, and adding
the "entry point" concept - to avoid a blow-up in the number of
codegen'd entry points that could lead to longer codegen times and/or
worse code because of inlining changes.
IMPALA-7331 (CHAR codegen support functions) is also fixed because
it was simpler to enable CHAR codegen within ScalarExpr than to carry
forward the exiting CHAR workarounds from ScalarFnCall. The
CHAR-specific codegen support required in the scalar expr subsystem is
very limited. StringVal intermediates are used everywhere. Only
SlotRef actually operates on the different tuple layout, and the
required codegen support for SlotRef already exists for UDA
intermediates anyway.
Testing:
* Ran exhaustive tests.
Perf:
* Ran a basic insert benchmark, which went from 10.1s to 7.6s
create table foo stored as parquet as
select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end
from tpch30_parquet.lineitem;
* Ran a basic CHAR expr test:
set num_nodes=1;
set mt_dop=1;
select count(*) from lineitem
where cast(l_linestatus as CHAR(2)) = 'O ' and
cast(l_returnflag as CHAR(2)) = 'N '
The time spent in the scan went from 520ms to 220ms.
* Added perf regression test to tpcds-insert, similar to the manual
benchmark.
* Ran single-node TPC-H with large and small scale factors, to estimate
impact on execution perf and query startup time, respectively.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 6.84 | -0.18% | 4.49 | -0.31% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.58 | 2.47 | +4.18% | 1.29% | 0.88% | 5 | +4.12% | 2.31 | 5.81 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 4.81 | 4.61 | +4.33% | 2.18% | 2.15% | 5 | +3.91% | 1.73 | 3.09 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 26.45 | 26.16 | +1.09% | 0.37% | 0.50% | 5 | +1.36% | 2.02 | 3.94 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 15.92 | 15.75 | +1.09% | 2.87% | 1.65% | 5 | +0.88% | 0.29 | 0.73 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.38 | 2.35 | +1.12% | 1.64% | 1.11% | 5 | +0.80% | 1.15 | 1.26 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.13% | 7.68% | 5.37% | 5 | -0.34% | -0.29 | 0.27 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 18.10 | 18.02 | +0.42% | 2.70% | 0.56% | 5 | +0.28% | 0.29 | 0.34 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 4.72 | 4.72 | -0.04% | 1.20% | 1.65% | 5 | +0.05% | 0.00 | -0.04 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.92 | 3.93 | -0.26% | 1.08% | 2.36% | 5 | +0.20% | 0.58 | -0.23 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.27 | 1.27 | -0.28% | 0.22% | 0.88% | 5 | +0.09% | 0.29 | -0.68 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.64 | 2.65 | -0.45% | 1.65% | 0.65% | 5 | -0.24% | -0.58 | -0.57 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 3.10 | 3.13 | -0.76% | 1.47% | 1.12% | 5 | -0.21% | -0.29 | -0.93 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 1.20 | 1.21 | -0.80% | 2.26% | 2.47% | 5 | -0.82% | -1.15 | -0.53 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 1.97 | 1.99 | -1.37% | 1.84% | 3.21% | 5 | -0.47% | -0.58 | -0.83 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 11.53 | 11.63 | -0.91% | 0.46% | 0.49% | 5 | -0.95% | -2.02 | -3.08 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 5.13 | 5.21 | -1.51% | 2.24% | 4.05% | 5 | -0.94% | -0.58 | -0.73 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 3.61 | 3.66 | -1.40% | 0.66% | 0.79% | 5 | -1.33% | -1.73 | -3.05 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 19.42 | 19.71 | -1.52% | 1.34% | 1.39% | 5 | -1.22% | -1.44 | -1.76 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 5.08 | 5.15 | -1.49% | 1.34% | 0.73% | 5 | -1.35% | -1.44 | -2.20 |
| TPCH(30) | TPCH-Q15 | parquet / none / none | 3.42 | 3.49 | -1.92% | 0.93% | 1.47% | 5 | -1.53% | -1.15 | -2.49 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.15 | 1.19 | -3.17% | 2.27% | 1.95% | 5 | -4.21% | -1.15 | -2.41 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 9.26 | 9.63 | -3.85% | 0.62% | 0.59% | 5 | -3.78% | -2.31 | -10.25 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
Cluster Name: UNKNOWN
Lab Run Info: UNKNOWN
Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE ()
Baseline Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE (2019-03-19)
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(2) | parquet / none / none | 0.90 | -0.08% | 0.80 | -0.05% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(2) | TPCH-Q18 | parquet / none / none | 1.22 | 1.19 | +1.93% | 3.81% | 4.46% | 20 | +3.34% | 1.62 | 1.46 |
| TPCH(2) | TPCH-Q10 | parquet / none / none | 0.74 | 0.73 | +1.97% | 3.36% | 2.94% | 20 | +0.97% | 1.88 | 1.95 |
| TPCH(2) | TPCH-Q11 | parquet / none / none | 0.49 | 0.48 | +1.91% | 6.19% | 4.64% | 20 | +0.25% | 0.95 | 1.09 |
| TPCH(2) | TPCH-Q4 | parquet / none / none | 0.43 | 0.43 | +1.99% | 6.26% | 5.86% | 20 | +0.15% | 0.92 | 1.03 |
| TPCH(2) | TPCH-Q15 | parquet / none / none | 0.50 | 0.49 | +1.82% | 7.32% | 6.35% | 20 | +0.26% | 1.01 | 0.83 |
| TPCH(2) | TPCH-Q1 | parquet / none / none | 0.98 | 0.97 | +0.79% | 4.64% | 2.73% | 20 | +0.36% | 0.77 | 0.65 |
| TPCH(2) | TPCH-Q19 | parquet / none / none | 0.83 | 0.83 | +0.65% | 3.33% | 2.80% | 20 | +0.44% | 2.18 | 0.67 |
| TPCH(2) | TPCH-Q14 | parquet / none / none | 0.62 | 0.62 | +0.97% | 2.86% | 1.00% | 20 | +0.04% | 0.13 | 1.42 |
| TPCH(2) | TPCH-Q3 | parquet / none / none | 0.88 | 0.87 | +0.57% | 2.17% | 1.74% | 20 | +0.29% | 1.15 | 0.92 |
| TPCH(2) | TPCH-Q12 | parquet / none / none | 0.53 | 0.53 | +0.27% | 4.58% | 5.78% | 20 | +0.46% | 1.47 | 0.16 |
| TPCH(2) | TPCH-Q17 | parquet / none / none | 0.72 | 0.72 | +0.15% | 3.64% | 5.55% | 20 | +0.21% | 0.86 | 0.10 |
| TPCH(2) | TPCH-Q21 | parquet / none / none | 2.05 | 2.05 | +0.21% | 1.99% | 2.37% | 20 | +0.01% | 0.25 | 0.30 |
| TPCH(2) | TPCH-Q5 | parquet / none / none | 1.28 | 1.27 | +0.24% | 1.61% | 1.80% | 20 | -0.02% | -0.57 | 0.44 |
| TPCH(2) | TPCH-Q13 | parquet / none / none | 1.27 | 1.27 | -0.34% | 1.69% | 1.83% | 20 | -0.20% | -1.65 | -0.61 |
| TPCH(2) | TPCH-Q7 | parquet / none / none | 1.72 | 1.73 | -0.55% | 2.40% | 1.69% | 20 | -0.03% | -0.42 | -0.83 |
| TPCH(2) | TPCH-Q8 | parquet / none / none | 1.27 | 1.28 | -0.68% | 3.10% | 3.89% | 20 | -0.06% | -0.54 | -0.62 |
| TPCH(2) | TPCH-Q6 | parquet / none / none | 0.36 | 0.36 | -0.84% | 0.79% | 3.51% | 20 | -0.07% | -0.36 | -1.04 |
| TPCH(2) | TPCH-Q2 | parquet / none / none | 0.65 | 0.65 | -1.17% | 4.76% | 5.99% | 20 | -0.05% | -0.25 | -0.69 |
| TPCH(2) | TPCH-Q9 | parquet / none / none | 1.59 | 1.62 | -2.01% | 1.45% | 5.12% | 20 | -0.16% | -1.24 | -1.69 |
| TPCH(2) | TPCH-Q20 | parquet / none / none | 0.68 | 0.69 | -1.73% | 4.35% | 4.43% | 20 | -0.49% | -1.74 | -1.25 |
| TPCH(2) | TPCH-Q22 | parquet / none / none | 0.38 | 0.40 | -2.89% | 7.42% | 6.39% | 20 | -0.21% | -0.66 | -1.34 |
| TPCH(2) | TPCH-Q16 | parquet / none / none | 0.59 | 0.62 | -4.01% | 6.33% | 5.83% | 20 | -4.72% | -1.39 | -2.13 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregator.cc
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/union-node.cc
M be/src/exprs/CMakeLists.txt
M be/src/exprs/agg-fn.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/case-expr.h
M be/src/exprs/compound-predicates.cc
M be/src/exprs/compound-predicates.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/literal.cc
M be/src/exprs/literal.h
M be/src/exprs/null-literal.cc
M be/src/exprs/null-literal.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/scalar-expr-ir.cc
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
A be/src/exprs/scalar-expr.inline.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
D be/src/exprs/slot-ref-ir.cc
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
R be/src/runtime/collection-value.cc
M be/src/runtime/collection-value.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tuple.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/util/tuple-row-compare.cc
M testdata/workloads/functional-query/queries/QueryTest/datastream-sender-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/disable-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
A testdata/workloads/tpcds-insert/queries/expr-insert.test
M tests/query_test/test_codegen.py
M tests/query_test/test_tpcds_queries.py
66 files changed, 838 insertions(+), 919 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12797/13
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 13
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 8:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/codegen/codegen-anyval.cc@37
PS8, Line 37:
nit: whitespace
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/codegen/llvm-codegen.h@44
PS8, Line 44: #include "runtime/collection-value.h"
Is this needed?
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.h
File be/src/exprs/scalar-expr.h:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.h@292
PS8, Line 292: /// TODO: not all root exprs need to be entry point, e.g. if only called from a
This TODO might make more sense in ScalarExpr::Create in scalar-expr.cc where the actual call with is_entry_point=true is.
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.cc
File be/src/exprs/scalar-expr.cc:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.cc@402
PS8, Line 402: // Ensure that Get*Val() can be called on each child by calling with
: // is_codegen_entry_point=true.
I'm a little confused by this comment - if I understand correctly, Get*Val() can always be called on any ScalarExpr since it can just fall back to the interpreted path, so the point of this is to make sure that when Get*Val() is called on a child it goes through the codegen'd path if possible?
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.inline.h
File be/src/exprs/scalar-expr.inline.h:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.inline.h@24
PS8, Line 24: typedef BooleanVal (*BooleanValWrapper)(ScalarExprEvaluator*, const TupleRow*);
These could be moved into the macro below
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-fn-call.cc@485
PS8, Line 485: \
nit: whitespace
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/udf/udf-internal.h
File be/src/udf/udf-internal.h:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/udf/udf-internal.h@300
PS8, Line 300: Also is denser
I find this comment confusing. Denser than what? (I assume you mean denser than the previous layout, but someone reading this for the first time wouldn't necessarily have that context)
http://gerrit.cloudera.org:8080/#/c/12797/8/tests/query_test/test_tpcds_queries.py
File tests/query_test/test_tpcds_queries.py:
http://gerrit.cloudera.org:8080/#/c/12797/8/tests/query_test/test_tpcds_queries.py@528
PS8, Line 528: def test_expr_insert(self, vector):
I think this test as is will not actually get run in any of our regular builds (something that I stumbled upon when working on https://gerrit.cloudera.org/#/c/12960/). I'm assuming that's not your intention.
The issue is that even in EXHAUSTIVE builds, we only run test cases that have a workload of 'functional-query' in exhaustive mode (see how we set the values of --exploration_strategy and --workload_exploration_strategy in bin/run-all-tests.sh), so the check in add_test_dimensions() that disables this in core mode effectively disables it entirely.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Apr 2019 19:59:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 12:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3168/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 16:23:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12797
to look at the new patch set (#8).
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Based on initial draft patch by Pooja Nilangekar.
Codegen'd expressions can be executed in two ways - either by
being called directly from a fully codegend function, or from
interpreted code via a function pointer (previously
ScalarFnCall::scalar_fn_wrapper_).
This change moves the function pointer from ScalarFnCall to its
base class ScalarExpr, so the full expr tree can be codegen'd, not
just the ScalarFnCall subtrees. The key refactoring and improvements
are:
* ScalarExpr::Get*Val() switches between interpreted and the codegen'd
function pointer code paths in an inline function, avoiding a
virtual function call to ScalarFnCal::Get*Val().
* Boilerplate logic is moved to ScalarExpr::GetCodegendComputeFn(),
which calls a virtual function GetCodegenComputeFnImpl().
* ScalarFnCall's logic for deciding whether to interpret or codegen is
better abstracted and exposed to ScalarExpr as IsInterpretable()
and ShouldCodegen() methods.
* The ScalarExpr::codegend_compute_fn_ function pointer is only
populated for expressions that are "codegen entry points". These
include the roots of expr trees and non-root expressions
where the parent expression calls Get*Val() from the
pseudo-codegend GetCodegendComputeFnWrapper().
* ScalarFnCall is always initialised for interpreted execution.
Otherwise the function pointer is needed for non-root expressions,
e.g. to support ScalarExprEvaluator::GetConstantVal().
* Latent bugs/gaps for codegen of CollectionVal are fixed. CollectionVal
is modified to use the StringVal memory layout to allow code sharing
with StringVal. These fixes allowed simplification of
IsNotEmptyPredicate codegen (from IMPALA-7657).
IMPALA-7331 (CHAR codegen support functions) is also fixed because
it was simpler to enable CHAR codegen within ScalarExpr than to carry
forward the exiting CHAR workarounds from ScalarFnCall. The
CHAR-specific codegen support required in the scalar expr subsystem is
very limited. StringVal intermediates are used everywhere. Only
SlotRef actually operates on the different tuple layout, and the
required codegen support for SlotRef already exists for UDA
intermediates anyway.
Testing:
* Ran exhaustive tests.
Perf:
* Ran a basic insert benchmark, which went from 10.1s to 7.6s
create table foo stored as parquet as
select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end
from tpch30_parquet.lineitem;
* Ran a basic CHAR expr test:
set num_nodes=1;
set mt_dop=1;
select count(*) from lineitem
where cast(l_linestatus as CHAR(2)) = 'O ' and
cast(l_returnflag as CHAR(2)) = 'N '
The time spent in the scan went from 520ms to 220ms.
* Added perf regression test to tpcds-insert, similar to the manual
benchmark.
* Ran single-node TPC-H with large and small scale factors, to estimate
impact on execution perf and query startup time, respectively.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 6.84 | -0.18% | 4.49 | -0.31% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.58 | 2.47 | +4.18% | 1.29% | 0.88% | 5 | +4.12% | 2.31 | 5.81 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 4.81 | 4.61 | +4.33% | 2.18% | 2.15% | 5 | +3.91% | 1.73 | 3.09 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 26.45 | 26.16 | +1.09% | 0.37% | 0.50% | 5 | +1.36% | 2.02 | 3.94 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 15.92 | 15.75 | +1.09% | 2.87% | 1.65% | 5 | +0.88% | 0.29 | 0.73 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.38 | 2.35 | +1.12% | 1.64% | 1.11% | 5 | +0.80% | 1.15 | 1.26 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.13% | 7.68% | 5.37% | 5 | -0.34% | -0.29 | 0.27 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 18.10 | 18.02 | +0.42% | 2.70% | 0.56% | 5 | +0.28% | 0.29 | 0.34 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 4.72 | 4.72 | -0.04% | 1.20% | 1.65% | 5 | +0.05% | 0.00 | -0.04 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.92 | 3.93 | -0.26% | 1.08% | 2.36% | 5 | +0.20% | 0.58 | -0.23 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.27 | 1.27 | -0.28% | 0.22% | 0.88% | 5 | +0.09% | 0.29 | -0.68 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.64 | 2.65 | -0.45% | 1.65% | 0.65% | 5 | -0.24% | -0.58 | -0.57 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 3.10 | 3.13 | -0.76% | 1.47% | 1.12% | 5 | -0.21% | -0.29 | -0.93 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 1.20 | 1.21 | -0.80% | 2.26% | 2.47% | 5 | -0.82% | -1.15 | -0.53 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 1.97 | 1.99 | -1.37% | 1.84% | 3.21% | 5 | -0.47% | -0.58 | -0.83 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 11.53 | 11.63 | -0.91% | 0.46% | 0.49% | 5 | -0.95% | -2.02 | -3.08 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 5.13 | 5.21 | -1.51% | 2.24% | 4.05% | 5 | -0.94% | -0.58 | -0.73 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 3.61 | 3.66 | -1.40% | 0.66% | 0.79% | 5 | -1.33% | -1.73 | -3.05 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 19.42 | 19.71 | -1.52% | 1.34% | 1.39% | 5 | -1.22% | -1.44 | -1.76 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 5.08 | 5.15 | -1.49% | 1.34% | 0.73% | 5 | -1.35% | -1.44 | -2.20 |
| TPCH(30) | TPCH-Q15 | parquet / none / none | 3.42 | 3.49 | -1.92% | 0.93% | 1.47% | 5 | -1.53% | -1.15 | -2.49 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.15 | 1.19 | -3.17% | 2.27% | 1.95% | 5 | -4.21% | -1.15 | -2.41 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 9.26 | 9.63 | -3.85% | 0.62% | 0.59% | 5 | -3.78% | -2.31 | -10.25 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
Cluster Name: UNKNOWN
Lab Run Info: UNKNOWN
Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE ()
Baseline Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE (2019-03-19)
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(2) | parquet / none / none | 0.90 | -0.08% | 0.80 | -0.05% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(2) | TPCH-Q18 | parquet / none / none | 1.22 | 1.19 | +1.93% | 3.81% | 4.46% | 20 | +3.34% | 1.62 | 1.46 |
| TPCH(2) | TPCH-Q10 | parquet / none / none | 0.74 | 0.73 | +1.97% | 3.36% | 2.94% | 20 | +0.97% | 1.88 | 1.95 |
| TPCH(2) | TPCH-Q11 | parquet / none / none | 0.49 | 0.48 | +1.91% | 6.19% | 4.64% | 20 | +0.25% | 0.95 | 1.09 |
| TPCH(2) | TPCH-Q4 | parquet / none / none | 0.43 | 0.43 | +1.99% | 6.26% | 5.86% | 20 | +0.15% | 0.92 | 1.03 |
| TPCH(2) | TPCH-Q15 | parquet / none / none | 0.50 | 0.49 | +1.82% | 7.32% | 6.35% | 20 | +0.26% | 1.01 | 0.83 |
| TPCH(2) | TPCH-Q1 | parquet / none / none | 0.98 | 0.97 | +0.79% | 4.64% | 2.73% | 20 | +0.36% | 0.77 | 0.65 |
| TPCH(2) | TPCH-Q19 | parquet / none / none | 0.83 | 0.83 | +0.65% | 3.33% | 2.80% | 20 | +0.44% | 2.18 | 0.67 |
| TPCH(2) | TPCH-Q14 | parquet / none / none | 0.62 | 0.62 | +0.97% | 2.86% | 1.00% | 20 | +0.04% | 0.13 | 1.42 |
| TPCH(2) | TPCH-Q3 | parquet / none / none | 0.88 | 0.87 | +0.57% | 2.17% | 1.74% | 20 | +0.29% | 1.15 | 0.92 |
| TPCH(2) | TPCH-Q12 | parquet / none / none | 0.53 | 0.53 | +0.27% | 4.58% | 5.78% | 20 | +0.46% | 1.47 | 0.16 |
| TPCH(2) | TPCH-Q17 | parquet / none / none | 0.72 | 0.72 | +0.15% | 3.64% | 5.55% | 20 | +0.21% | 0.86 | 0.10 |
| TPCH(2) | TPCH-Q21 | parquet / none / none | 2.05 | 2.05 | +0.21% | 1.99% | 2.37% | 20 | +0.01% | 0.25 | 0.30 |
| TPCH(2) | TPCH-Q5 | parquet / none / none | 1.28 | 1.27 | +0.24% | 1.61% | 1.80% | 20 | -0.02% | -0.57 | 0.44 |
| TPCH(2) | TPCH-Q13 | parquet / none / none | 1.27 | 1.27 | -0.34% | 1.69% | 1.83% | 20 | -0.20% | -1.65 | -0.61 |
| TPCH(2) | TPCH-Q7 | parquet / none / none | 1.72 | 1.73 | -0.55% | 2.40% | 1.69% | 20 | -0.03% | -0.42 | -0.83 |
| TPCH(2) | TPCH-Q8 | parquet / none / none | 1.27 | 1.28 | -0.68% | 3.10% | 3.89% | 20 | -0.06% | -0.54 | -0.62 |
| TPCH(2) | TPCH-Q6 | parquet / none / none | 0.36 | 0.36 | -0.84% | 0.79% | 3.51% | 20 | -0.07% | -0.36 | -1.04 |
| TPCH(2) | TPCH-Q2 | parquet / none / none | 0.65 | 0.65 | -1.17% | 4.76% | 5.99% | 20 | -0.05% | -0.25 | -0.69 |
| TPCH(2) | TPCH-Q9 | parquet / none / none | 1.59 | 1.62 | -2.01% | 1.45% | 5.12% | 20 | -0.16% | -1.24 | -1.69 |
| TPCH(2) | TPCH-Q20 | parquet / none / none | 0.68 | 0.69 | -1.73% | 4.35% | 4.43% | 20 | -0.49% | -1.74 | -1.25 |
| TPCH(2) | TPCH-Q22 | parquet / none / none | 0.38 | 0.40 | -2.89% | 7.42% | 6.39% | 20 | -0.21% | -0.66 | -1.34 |
| TPCH(2) | TPCH-Q16 | parquet / none / none | 0.59 | 0.62 | -4.01% | 6.33% | 5.83% | 20 | -4.72% | -1.39 | -2.13 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregator.cc
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/union-node.cc
M be/src/exprs/CMakeLists.txt
M be/src/exprs/agg-fn.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/case-expr.h
M be/src/exprs/compound-predicates.cc
M be/src/exprs/compound-predicates.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/literal.cc
M be/src/exprs/literal.h
M be/src/exprs/null-literal.cc
M be/src/exprs/null-literal.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/scalar-expr-ir.cc
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
A be/src/exprs/scalar-expr.inline.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
D be/src/exprs/slot-ref-ir.cc
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
R be/src/runtime/collection-value.cc
M be/src/runtime/collection-value.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tuple.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/util/tuple-row-compare.cc
M testdata/workloads/functional-query/queries/QueryTest/datastream-sender-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/disable-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
A testdata/workloads/tpcds-insert/queries/expr-insert.test
M tests/query_test/test_codegen.py
M tests/query_test/test_tpcds_queries.py
66 files changed, 790 insertions(+), 886 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12797/8
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Mar 2019 20:27:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has removed a vote on this change.
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Removed Verified-1 by Impala Public Jenkins <im...@cloudera.com>
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 16
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2667/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 06 Apr 2019 01:22:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 5:
This touches some of the same code as the big DATE patch, so I think I'll plan on merging this after DATE so that I can do the appropriate fixups. The changes to implement DATE here should be fairly mechanical though, so this is reviewable now:
https://gerrit.cloudera.org/#/c/12481/
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Mar 2019 20:34:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 12:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exec/grouping-aggregator.cc
File be/src/exec/grouping-aggregator.cc:
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exec/grouping-aggregator.cc@119
PS12, Line 119: intermediate_row_desc_, /* is_entry_point */ false, state));
> nit: indentation
Done - ran clang-format on it.
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h
File be/src/exprs/scalar-expr.h:
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@110
PS12, Line 110: interpreted code.
> (e.g. an operator which doesn't support codegen yet).
Done
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@117
PS12, Line 117: /// TODO: Fix subclasses which call GetCodegendComputeFnWrapper() to not call interpreted
: /// functions.
> Stale ?
Still relevant, but could be clarified. Updated comment.
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@170
PS12, Line 170: /// compilation. Returns error status on failure.
> Can you please also add something similar to below:
Done
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@246
PS12, Line 246: for each
> to be overridden by
Done
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@388
PS12, Line 388: If this is true, then after the expressions are codegen'd,
: /// then 'codegend_compute_fn_' is non-NULL.
> If this is true, 'codegend_compute_fn_' will be set to the JIT'd function p
Done
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.cc
File be/src/exprs/scalar-expr.cc:
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.cc@86
PS12, Line 86: ///
> nit: //
Done
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.inline.h
File be/src/exprs/scalar-expr.inline.h:
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.inline.h@24
PS12, Line 24: //
> nit: ///
Done
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.inline.h@28
PS12, Line 28: ScalarFnCall
> ScalarExpr
Done
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-fn-call.cc@95
PS12, Line 95: // CHAR or VARCHAR are not supported as input arguments or return values for UDFs.
> stale ?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 11 May 2019 00:42:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 4:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/conditional-functions-ir.cc
File be/src/exprs/conditional-functions-ir.cc:
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/conditional-functions-ir.cc@94
PS4, Line 94: type IfExpr::Get##type##Interpreted(ScalarExprEvaluator* eval, const TupleRow* row) const { \
line too long (95 > 90)
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/is-not-empty-predicate.h
File be/src/exprs/is-not-empty-predicate.h:
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/is-not-empty-predicate.h@34
PS4, Line 34: virtual Status GetCodegendComputeFnImpl(LlvmCodeGen* codegen, llvm::Function** fn) override;
line too long (94 > 90)
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/scalar-fn-call.cc@129
PS4, Line 129: // For IR UDF, the loading of the Init() and CloseContext() functions is deferred until
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/valid-tuple-id.h
File be/src/exprs/valid-tuple-id.h:
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/valid-tuple-id.h@44
PS4, Line 44: virtual IntVal GetIntValInterpreted(ScalarExprEvaluator*, const TupleRow*) const override;
line too long (92 > 90)
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/valid-tuple-id.cc
File be/src/exprs/valid-tuple-id.cc:
http://gerrit.cloudera.org:8080/#/c/12797/4/be/src/exprs/valid-tuple-id.cc@51
PS4, Line 51: IntVal ValidTupleIdExpr::GetIntValInterpreted(ScalarExprEvaluator* eval, const TupleRow* row) const {
line too long (101 > 90)
http://gerrit.cloudera.org:8080/#/c/12797/4/tests/query_test/test_codegen.py
File tests/query_test/test_codegen.py:
http://gerrit.cloudera.org:8080/#/c/12797/4/tests/query_test/test_codegen.py@77
PS4, Line 77: ;
flake8: E703 statement ends with a semicolon
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Mar 2019 15:57:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 13: Code-Review+2
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-fn-call.cc@143
PS12, Line 143: // staging_input_vals, which will be reused across calls to scalar_fn_ on the
: // interpreted code path..
: vector<AnyVal*>* input_vals = fn_ctx->impl()->staging_input_vals();
: for (int i = 0; i < NumFixedArgs(); ++i) {
: AnyVal* input_val;
: RETURN_IF_ERROR(AllocateAnyVal(state, eval->expr_perm_pool(), children_[i]->type(),
: "Could not allocate expression value", &input_val));
: input_vals->push_back(input_val);
: }
:
> Yes, the problem I ran into is that code like the below code to GetConstVal
I suppose an alternative is to do:
bool is_interpreted = codegend_compute_fn_ == nullptr;
Of course, this makes assumption about the order in which codegend_compute_fn_ is populated and when this function is called so it may add unnecessary complication. So, I think it's okay to leave it as-is but would be nice to briefly document the reasoning behind it.
http://gerrit.cloudera.org:8080/#/c/12797/13/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/13/be/src/exprs/scalar-fn-call.cc@142
PS13, Line 142: We're in the interpreted path (i.e. no JIT)
Prepare input_vals in case the interpreted path is invoked during initialization (e.g. GetConstantValue() below).
http://gerrit.cloudera.org:8080/#/c/12797/13/be/src/exprs/scalar-fn-call.cc@144
PS13, Line 144: .
nit: extra .
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 13
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2019 21:30:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12797
to look at the new patch set (#12).
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Based on initial draft patch by Pooja Nilangekar.
Codegen'd expressions can be executed in two ways - either by
being called directly from a fully codegend function, or from
interpreted code via a function pointer (previously
ScalarFnCall::scalar_fn_wrapper_).
This change moves the function pointer from ScalarFnCall to its
base class ScalarExpr, so the full expr tree can be codegen'd, not
just the ScalarFnCall subtrees. The key refactoring and improvements
are:
* ScalarExpr::Get*Val() switches between interpreted and the codegen'd
function pointer code paths in an inline function, avoiding a
virtual function call to ScalarFnCal::Get*Val().
* Boilerplate logic is moved to ScalarExpr::GetCodegendComputeFn(),
which calls a virtual function GetCodegenComputeFnImpl().
* ScalarFnCall's logic for deciding whether to interpret or codegen is
better abstracted and exposed to ScalarExpr as IsInterpretable()
and ShouldCodegen() methods.
* The ScalarExpr::codegend_compute_fn_ function pointer is only
populated for expressions that are "codegen entry points". These
include the roots of expr trees and non-root expressions
where the parent expression calls Get*Val() from the
pseudo-codegend GetCodegendComputeFnWrapper().
* ScalarFnCall is always initialised for interpreted execution.
Otherwise the function pointer is needed for non-root expressions,
e.g. to support ScalarExprEvaluator::GetConstantVal().
* Latent bugs/gaps for codegen of CollectionVal are fixed. CollectionVal
is modified to use the StringVal memory layout to allow code sharing
with StringVal. These fixes allowed simplification of
IsNotEmptyPredicate codegen (from IMPALA-7657).
I chose to tackle two problems in one change - adding support for
generating codegen'd function pointers for all ScalarExprs, and adding
the "entry point" concept - to avoid a blow-up in the number of
codegen'd entry points that could lead to longer codegen times and/or
worse code because of inlining changes.
IMPALA-7331 (CHAR codegen support functions) is also fixed because
it was simpler to enable CHAR codegen within ScalarExpr than to carry
forward the exiting CHAR workarounds from ScalarFnCall. The
CHAR-specific codegen support required in the scalar expr subsystem is
very limited. StringVal intermediates are used everywhere. Only
SlotRef actually operates on the different tuple layout, and the
required codegen support for SlotRef already exists for UDA
intermediates anyway.
Testing:
* Ran exhaustive tests.
Perf:
* Ran a basic insert benchmark, which went from 10.1s to 7.6s
create table foo stored as parquet as
select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end
from tpch30_parquet.lineitem;
* Ran a basic CHAR expr test:
set num_nodes=1;
set mt_dop=1;
select count(*) from lineitem
where cast(l_linestatus as CHAR(2)) = 'O ' and
cast(l_returnflag as CHAR(2)) = 'N '
The time spent in the scan went from 520ms to 220ms.
* Added perf regression test to tpcds-insert, similar to the manual
benchmark.
* Ran single-node TPC-H with large and small scale factors, to estimate
impact on execution perf and query startup time, respectively.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 6.84 | -0.18% | 4.49 | -0.31% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.58 | 2.47 | +4.18% | 1.29% | 0.88% | 5 | +4.12% | 2.31 | 5.81 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 4.81 | 4.61 | +4.33% | 2.18% | 2.15% | 5 | +3.91% | 1.73 | 3.09 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 26.45 | 26.16 | +1.09% | 0.37% | 0.50% | 5 | +1.36% | 2.02 | 3.94 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 15.92 | 15.75 | +1.09% | 2.87% | 1.65% | 5 | +0.88% | 0.29 | 0.73 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.38 | 2.35 | +1.12% | 1.64% | 1.11% | 5 | +0.80% | 1.15 | 1.26 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.13% | 7.68% | 5.37% | 5 | -0.34% | -0.29 | 0.27 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 18.10 | 18.02 | +0.42% | 2.70% | 0.56% | 5 | +0.28% | 0.29 | 0.34 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 4.72 | 4.72 | -0.04% | 1.20% | 1.65% | 5 | +0.05% | 0.00 | -0.04 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.92 | 3.93 | -0.26% | 1.08% | 2.36% | 5 | +0.20% | 0.58 | -0.23 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.27 | 1.27 | -0.28% | 0.22% | 0.88% | 5 | +0.09% | 0.29 | -0.68 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.64 | 2.65 | -0.45% | 1.65% | 0.65% | 5 | -0.24% | -0.58 | -0.57 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 3.10 | 3.13 | -0.76% | 1.47% | 1.12% | 5 | -0.21% | -0.29 | -0.93 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 1.20 | 1.21 | -0.80% | 2.26% | 2.47% | 5 | -0.82% | -1.15 | -0.53 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 1.97 | 1.99 | -1.37% | 1.84% | 3.21% | 5 | -0.47% | -0.58 | -0.83 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 11.53 | 11.63 | -0.91% | 0.46% | 0.49% | 5 | -0.95% | -2.02 | -3.08 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 5.13 | 5.21 | -1.51% | 2.24% | 4.05% | 5 | -0.94% | -0.58 | -0.73 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 3.61 | 3.66 | -1.40% | 0.66% | 0.79% | 5 | -1.33% | -1.73 | -3.05 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 19.42 | 19.71 | -1.52% | 1.34% | 1.39% | 5 | -1.22% | -1.44 | -1.76 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 5.08 | 5.15 | -1.49% | 1.34% | 0.73% | 5 | -1.35% | -1.44 | -2.20 |
| TPCH(30) | TPCH-Q15 | parquet / none / none | 3.42 | 3.49 | -1.92% | 0.93% | 1.47% | 5 | -1.53% | -1.15 | -2.49 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.15 | 1.19 | -3.17% | 2.27% | 1.95% | 5 | -4.21% | -1.15 | -2.41 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 9.26 | 9.63 | -3.85% | 0.62% | 0.59% | 5 | -3.78% | -2.31 | -10.25 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
Cluster Name: UNKNOWN
Lab Run Info: UNKNOWN
Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE ()
Baseline Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE (2019-03-19)
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(2) | parquet / none / none | 0.90 | -0.08% | 0.80 | -0.05% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(2) | TPCH-Q18 | parquet / none / none | 1.22 | 1.19 | +1.93% | 3.81% | 4.46% | 20 | +3.34% | 1.62 | 1.46 |
| TPCH(2) | TPCH-Q10 | parquet / none / none | 0.74 | 0.73 | +1.97% | 3.36% | 2.94% | 20 | +0.97% | 1.88 | 1.95 |
| TPCH(2) | TPCH-Q11 | parquet / none / none | 0.49 | 0.48 | +1.91% | 6.19% | 4.64% | 20 | +0.25% | 0.95 | 1.09 |
| TPCH(2) | TPCH-Q4 | parquet / none / none | 0.43 | 0.43 | +1.99% | 6.26% | 5.86% | 20 | +0.15% | 0.92 | 1.03 |
| TPCH(2) | TPCH-Q15 | parquet / none / none | 0.50 | 0.49 | +1.82% | 7.32% | 6.35% | 20 | +0.26% | 1.01 | 0.83 |
| TPCH(2) | TPCH-Q1 | parquet / none / none | 0.98 | 0.97 | +0.79% | 4.64% | 2.73% | 20 | +0.36% | 0.77 | 0.65 |
| TPCH(2) | TPCH-Q19 | parquet / none / none | 0.83 | 0.83 | +0.65% | 3.33% | 2.80% | 20 | +0.44% | 2.18 | 0.67 |
| TPCH(2) | TPCH-Q14 | parquet / none / none | 0.62 | 0.62 | +0.97% | 2.86% | 1.00% | 20 | +0.04% | 0.13 | 1.42 |
| TPCH(2) | TPCH-Q3 | parquet / none / none | 0.88 | 0.87 | +0.57% | 2.17% | 1.74% | 20 | +0.29% | 1.15 | 0.92 |
| TPCH(2) | TPCH-Q12 | parquet / none / none | 0.53 | 0.53 | +0.27% | 4.58% | 5.78% | 20 | +0.46% | 1.47 | 0.16 |
| TPCH(2) | TPCH-Q17 | parquet / none / none | 0.72 | 0.72 | +0.15% | 3.64% | 5.55% | 20 | +0.21% | 0.86 | 0.10 |
| TPCH(2) | TPCH-Q21 | parquet / none / none | 2.05 | 2.05 | +0.21% | 1.99% | 2.37% | 20 | +0.01% | 0.25 | 0.30 |
| TPCH(2) | TPCH-Q5 | parquet / none / none | 1.28 | 1.27 | +0.24% | 1.61% | 1.80% | 20 | -0.02% | -0.57 | 0.44 |
| TPCH(2) | TPCH-Q13 | parquet / none / none | 1.27 | 1.27 | -0.34% | 1.69% | 1.83% | 20 | -0.20% | -1.65 | -0.61 |
| TPCH(2) | TPCH-Q7 | parquet / none / none | 1.72 | 1.73 | -0.55% | 2.40% | 1.69% | 20 | -0.03% | -0.42 | -0.83 |
| TPCH(2) | TPCH-Q8 | parquet / none / none | 1.27 | 1.28 | -0.68% | 3.10% | 3.89% | 20 | -0.06% | -0.54 | -0.62 |
| TPCH(2) | TPCH-Q6 | parquet / none / none | 0.36 | 0.36 | -0.84% | 0.79% | 3.51% | 20 | -0.07% | -0.36 | -1.04 |
| TPCH(2) | TPCH-Q2 | parquet / none / none | 0.65 | 0.65 | -1.17% | 4.76% | 5.99% | 20 | -0.05% | -0.25 | -0.69 |
| TPCH(2) | TPCH-Q9 | parquet / none / none | 1.59 | 1.62 | -2.01% | 1.45% | 5.12% | 20 | -0.16% | -1.24 | -1.69 |
| TPCH(2) | TPCH-Q20 | parquet / none / none | 0.68 | 0.69 | -1.73% | 4.35% | 4.43% | 20 | -0.49% | -1.74 | -1.25 |
| TPCH(2) | TPCH-Q22 | parquet / none / none | 0.38 | 0.40 | -2.89% | 7.42% | 6.39% | 20 | -0.21% | -0.66 | -1.34 |
| TPCH(2) | TPCH-Q16 | parquet / none / none | 0.59 | 0.62 | -4.01% | 6.33% | 5.83% | 20 | -4.72% | -1.39 | -2.13 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregator.cc
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/union-node.cc
M be/src/exprs/CMakeLists.txt
M be/src/exprs/agg-fn.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/case-expr.h
M be/src/exprs/compound-predicates.cc
M be/src/exprs/compound-predicates.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/literal.cc
M be/src/exprs/literal.h
M be/src/exprs/null-literal.cc
M be/src/exprs/null-literal.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/scalar-expr-ir.cc
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
A be/src/exprs/scalar-expr.inline.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
D be/src/exprs/slot-ref-ir.cc
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
R be/src/runtime/collection-value.cc
M be/src/runtime/collection-value.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tuple.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/util/tuple-row-compare.cc
M testdata/workloads/functional-query/queries/QueryTest/datastream-sender-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/disable-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
A testdata/workloads/tpcds-insert/queries/expr-insert.test
M tests/query_test/test_codegen.py
M tests/query_test/test_tpcds_queries.py
66 files changed, 828 insertions(+), 916 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12797/12
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 8: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc@483
PS6, Line 483: #pragma push_macro("GET
> I like this idea. Went ahead with it as you suggested. It was enough to tur
Thanks for the change, less lines is good news!
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 10 Apr 2019 10:19:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2862/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 11
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 23 Apr 2019 19:25:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 12:
(12 comments)
Thanks for the explanation. Can you please answer the question in scalar-fn-call.cc. Otherwise, I think this can be +2'ed.
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exec/grouping-aggregator.cc
File be/src/exec/grouping-aggregator.cc:
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exec/grouping-aggregator.cc@119
PS12, Line 119: intermediate_row_desc_, /* is_entry_point */ false, state));
nit: indentation
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h
File be/src/exprs/scalar-expr.h:
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@110
PS12, Line 110: interpreted code.
(e.g. an operator which doesn't support codegen yet).
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@117
PS12, Line 117: /// TODO: Fix subclasses which call GetCodegendComputeFnWrapper() to not call interpreted
: /// functions.
Stale ?
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@170
PS12, Line 170: /// compilation. Returns error status on failure.
Can you please also add something similar to below:
This function is invoked either by some other codegen functions (e.g. the codegen code of a join) or by RuntimeState::CodegenScalarExprs() which is called from FIS::Open() before LLVM compilation. These two call sites correspond to the two usage patterns in the class comment.
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@246
PS12, Line 246: for each
to be overridden by
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.h@388
PS12, Line 388: If this is true, then after the expressions are codegen'd,
: /// then 'codegend_compute_fn_' is non-NULL.
If this is true, 'codegend_compute_fn_' will be set to the JIT'd function produced by GetCodegendComputeFn() after LLVM compilation.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.cc
File be/src/exprs/scalar-expr.cc:
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.cc@284
PS10, Line 284:
> This assumption is false - GetCodegendComputeFnWrapper() generates
> a codegen'd function that calls the interpreted path of its child.
> We need to generate entry points there to avoid a perf cliff where
> the whole subtree doesn't get codegen'd (which would be a
> regression from the old behaviour too).
>
That's true. I suppose this mostly affects ScalarFnCall, right ? Before this change, if the expression is not codegen'd, its children are not codegen'd unless the children are ScalarFnCall. In that sense, that'll be a regression. Are there other cases which I may have missed ?
>
> ScalarExpr doesn't know the context in which it is used. E.g. if
> it's the child of an AggregateExpr, then it's not an entry point
> but doesn't know that based on local information. There are
> actually a bunch more places where the expr is always called from
> codegen'd code and we could save some overhead, but I didn't go
> through and find them all.
I see. I can see an alternate implementation is to revert the polarity of the "is_entry_point" flag to false by default and makes the owners of expressions to explicitly set 'is_entry_point' when creating / initialization the expressions. Those owners most likely include some operators which don't yet support codegen but I haven't looked through the entire code to see if there could be other non-trivial cases.
On the other hand, this may make it more likely to regress if we miss some cases as ScalarFnCall currently codegen all the time regardless of the callers. I suppose we kind of need to go through all owners of expressions and mark those which don't need entry points when addressing the TODO in Create() above.
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.cc
File be/src/exprs/scalar-expr.cc:
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.cc@86
PS12, Line 86: ///
nit: //
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.inline.h
File be/src/exprs/scalar-expr.inline.h:
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.inline.h@24
PS12, Line 24: //
nit: ///
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-expr.inline.h@28
PS12, Line 28: ScalarFnCall
ScalarExpr
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-fn-call.cc@95
PS12, Line 95: // CHAR or VARCHAR are not supported as input arguments or return values for UDFs.
stale ?
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-fn-call.cc@143
PS12, Line 143: // We're in the interpreted path (i.e. no JIT). Populate our FunctionContext's
: // staging_input_vals, which will be reused across calls to scalar_fn_ on the
: // interpreted code path..
: vector<AnyVal*>* input_vals = fn_ctx->impl()->staging_input_vals();
: for (int i = 0; i < NumFixedArgs(); ++i) {
: AnyVal* input_val;
: RETURN_IF_ERROR(AllocateAnyVal(state, eval->expr_perm_pool(), children_[i]->type(),
: "Could not allocate expression value", &input_val));
: input_vals->push_back(input_val);
: }
Not sure I understand this part. Does it mean that we will do this initialization for the interpreted code path even if codegen succeeded ?
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 23:07:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 16: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 16
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2019 21:48:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2714/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 09 Apr 2019 23:05:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 8:
(8 comments)
Thanks for the comments. All good points.
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/codegen/codegen-anyval.cc@37
PS8, Line 37:
> nit: whitespace
Done
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/codegen/llvm-codegen.h@44
PS8, Line 44: #include "runtime/collection-value.h"
> Is this needed?
moved to .cc
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.h
File be/src/exprs/scalar-expr.h:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.h@292
PS8, Line 292: /// TODO: not all root exprs need to be entry point, e.g. if only called from a
> This TODO might make more sense in ScalarExpr::Create in scalar-expr.cc whe
Done
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.cc
File be/src/exprs/scalar-expr.cc:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.cc@402
PS8, Line 402: // Ensure that Get*Val() can be called on each child by calling with
: // is_codegen_entry_point=true.
> I'm a little confused by this comment - if I understand correctly, Get*Val(
Yeah that's true. I missed updating this comment - an earlier iteration of the patch tried to have some rules about when it was valid to call a subexpr.
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.inline.h
File be/src/exprs/scalar-expr.inline.h:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-expr.inline.h@24
PS8, Line 24: typedef BooleanVal (*BooleanValWrapper)(ScalarExprEvaluator*, const TupleRow*);
> These could be moved into the macro below
Done
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/exprs/scalar-fn-call.cc@485
PS8, Line 485: \
> nit: whitespace
Done
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/udf/udf-internal.h
File be/src/udf/udf-internal.h:
http://gerrit.cloudera.org:8080/#/c/12797/8/be/src/udf/udf-internal.h@300
PS8, Line 300: Also is denser
> I find this comment confusing. Denser than what? (I assume you mean denser
Done
http://gerrit.cloudera.org:8080/#/c/12797/8/tests/query_test/test_tpcds_queries.py
File tests/query_test/test_tpcds_queries.py:
http://gerrit.cloudera.org:8080/#/c/12797/8/tests/query_test/test_tpcds_queries.py@528
PS8, Line 528: def test_expr_insert(self, vector):
> I think this test as is will not actually get run in any of our regular bui
Yeah this is true, test_partitioned_insert is actually broken. I did run this test directly and it passes. Filing IMPALA-8410 since I think there's probably some thought required to do it the right way, given the concern about runtime above.
I'm open to fixing in this patch, since I'm adding the test, but it seems easier to separate it.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Apr 2019 21:41:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 15: Code-Review+2
(3 comments)
carry
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/12/be/src/exprs/scalar-fn-call.cc@143
PS12, Line 143: // this function is invoked. staging_input_vals is preallocated here
: // so they can be reused across calls. If we have a codegen'd entry point
: // for this expression, allocating these input values may be unnecessary,
: // but they only add a small constant overhead on top of the ScalarExpr tree, so
: // we always allocate them for simplicity.
: vector<AnyVal*>* input_vals = fn_ctx->impl()->staging_input_vals();
: for (int i = 0; i < NumFixedArgs(); ++i) {
: AnyVal* input_val;
: RETURN_IF_ERROR(AllocateAnyVal(state, eval->expr_perm_pool(), children_[i]->type(),
:
> I suppose an alternative is to do:
Updated the comment (it was a bit stale anyway).
http://gerrit.cloudera.org:8080/#/c/12797/13/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/13/be/src/exprs/scalar-fn-call.cc@142
PS13, Line 142: Prepare staging_input_vals in case the inte
> Prepare input_vals in case the interpreted path is invoked during initializ
Done
http://gerrit.cloudera.org:8080/#/c/12797/13/be/src/exprs/scalar-fn-call.cc@144
PS13, Line 144: a
> nit: extra .
Done
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 15
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2019 21:48:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12797
to look at the new patch set (#14).
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Based on initial draft patch by Pooja Nilangekar.
Codegen'd expressions can be executed in two ways - either by
being called directly from a fully codegend function, or from
interpreted code via a function pointer (previously
ScalarFnCall::scalar_fn_wrapper_).
This change moves the function pointer from ScalarFnCall to its
base class ScalarExpr, so the full expr tree can be codegen'd, not
just the ScalarFnCall subtrees. The key refactoring and improvements
are:
* ScalarExpr::Get*Val() switches between interpreted and the codegen'd
function pointer code paths in an inline function, avoiding a
virtual function call to ScalarFnCal::Get*Val().
* Boilerplate logic is moved to ScalarExpr::GetCodegendComputeFn(),
which calls a virtual function GetCodegenComputeFnImpl().
* ScalarFnCall's logic for deciding whether to interpret or codegen is
better abstracted and exposed to ScalarExpr as IsInterpretable()
and ShouldCodegen() methods.
* The ScalarExpr::codegend_compute_fn_ function pointer is only
populated for expressions that are "codegen entry points". These
include the roots of expr trees and non-root expressions
where the parent expression calls Get*Val() from the
pseudo-codegend GetCodegendComputeFnWrapper().
* ScalarFnCall is always initialised for interpreted execution.
Otherwise the function pointer is needed for non-root expressions,
e.g. to support ScalarExprEvaluator::GetConstantVal().
* Latent bugs/gaps for codegen of CollectionVal are fixed. CollectionVal
is modified to use the StringVal memory layout to allow code sharing
with StringVal. These fixes allowed simplification of
IsNotEmptyPredicate codegen (from IMPALA-7657).
I chose to tackle two problems in one change - adding support for
generating codegen'd function pointers for all ScalarExprs, and adding
the "entry point" concept - to avoid a blow-up in the number of
codegen'd entry points that could lead to longer codegen times and/or
worse code because of inlining changes.
IMPALA-7331 (CHAR codegen support functions) is also fixed because
it was simpler to enable CHAR codegen within ScalarExpr than to carry
forward the exiting CHAR workarounds from ScalarFnCall. The
CHAR-specific codegen support required in the scalar expr subsystem is
very limited. StringVal intermediates are used everywhere. Only
SlotRef actually operates on the different tuple layout, and the
required codegen support for SlotRef already exists for UDA
intermediates anyway.
Testing:
* Ran exhaustive tests.
Perf:
* Ran a basic insert benchmark, which went from 10.1s to 7.6s
create table foo stored as parquet as
select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end
from tpch30_parquet.lineitem;
* Ran a basic CHAR expr test:
set num_nodes=1;
set mt_dop=1;
select count(*) from lineitem
where cast(l_linestatus as CHAR(2)) = 'O ' and
cast(l_returnflag as CHAR(2)) = 'N '
The time spent in the scan went from 520ms to 220ms.
* Added perf regression test to tpcds-insert, similar to the manual
benchmark.
* Ran single-node TPC-H with large and small scale factors, to estimate
impact on execution perf and query startup time, respectively.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 6.84 | -0.18% | 4.49 | -0.31% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.58 | 2.47 | +4.18% | 1.29% | 0.88% | 5 | +4.12% | 2.31 | 5.81 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 4.81 | 4.61 | +4.33% | 2.18% | 2.15% | 5 | +3.91% | 1.73 | 3.09 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 26.45 | 26.16 | +1.09% | 0.37% | 0.50% | 5 | +1.36% | 2.02 | 3.94 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 15.92 | 15.75 | +1.09% | 2.87% | 1.65% | 5 | +0.88% | 0.29 | 0.73 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.38 | 2.35 | +1.12% | 1.64% | 1.11% | 5 | +0.80% | 1.15 | 1.26 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.13% | 7.68% | 5.37% | 5 | -0.34% | -0.29 | 0.27 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 18.10 | 18.02 | +0.42% | 2.70% | 0.56% | 5 | +0.28% | 0.29 | 0.34 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 4.72 | 4.72 | -0.04% | 1.20% | 1.65% | 5 | +0.05% | 0.00 | -0.04 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.92 | 3.93 | -0.26% | 1.08% | 2.36% | 5 | +0.20% | 0.58 | -0.23 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.27 | 1.27 | -0.28% | 0.22% | 0.88% | 5 | +0.09% | 0.29 | -0.68 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.64 | 2.65 | -0.45% | 1.65% | 0.65% | 5 | -0.24% | -0.58 | -0.57 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 3.10 | 3.13 | -0.76% | 1.47% | 1.12% | 5 | -0.21% | -0.29 | -0.93 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 1.20 | 1.21 | -0.80% | 2.26% | 2.47% | 5 | -0.82% | -1.15 | -0.53 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 1.97 | 1.99 | -1.37% | 1.84% | 3.21% | 5 | -0.47% | -0.58 | -0.83 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 11.53 | 11.63 | -0.91% | 0.46% | 0.49% | 5 | -0.95% | -2.02 | -3.08 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 5.13 | 5.21 | -1.51% | 2.24% | 4.05% | 5 | -0.94% | -0.58 | -0.73 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 3.61 | 3.66 | -1.40% | 0.66% | 0.79% | 5 | -1.33% | -1.73 | -3.05 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 19.42 | 19.71 | -1.52% | 1.34% | 1.39% | 5 | -1.22% | -1.44 | -1.76 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 5.08 | 5.15 | -1.49% | 1.34% | 0.73% | 5 | -1.35% | -1.44 | -2.20 |
| TPCH(30) | TPCH-Q15 | parquet / none / none | 3.42 | 3.49 | -1.92% | 0.93% | 1.47% | 5 | -1.53% | -1.15 | -2.49 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.15 | 1.19 | -3.17% | 2.27% | 1.95% | 5 | -4.21% | -1.15 | -2.41 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 9.26 | 9.63 | -3.85% | 0.62% | 0.59% | 5 | -3.78% | -2.31 | -10.25 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
Cluster Name: UNKNOWN
Lab Run Info: UNKNOWN
Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE ()
Baseline Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE (2019-03-19)
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(2) | parquet / none / none | 0.90 | -0.08% | 0.80 | -0.05% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(2) | TPCH-Q18 | parquet / none / none | 1.22 | 1.19 | +1.93% | 3.81% | 4.46% | 20 | +3.34% | 1.62 | 1.46 |
| TPCH(2) | TPCH-Q10 | parquet / none / none | 0.74 | 0.73 | +1.97% | 3.36% | 2.94% | 20 | +0.97% | 1.88 | 1.95 |
| TPCH(2) | TPCH-Q11 | parquet / none / none | 0.49 | 0.48 | +1.91% | 6.19% | 4.64% | 20 | +0.25% | 0.95 | 1.09 |
| TPCH(2) | TPCH-Q4 | parquet / none / none | 0.43 | 0.43 | +1.99% | 6.26% | 5.86% | 20 | +0.15% | 0.92 | 1.03 |
| TPCH(2) | TPCH-Q15 | parquet / none / none | 0.50 | 0.49 | +1.82% | 7.32% | 6.35% | 20 | +0.26% | 1.01 | 0.83 |
| TPCH(2) | TPCH-Q1 | parquet / none / none | 0.98 | 0.97 | +0.79% | 4.64% | 2.73% | 20 | +0.36% | 0.77 | 0.65 |
| TPCH(2) | TPCH-Q19 | parquet / none / none | 0.83 | 0.83 | +0.65% | 3.33% | 2.80% | 20 | +0.44% | 2.18 | 0.67 |
| TPCH(2) | TPCH-Q14 | parquet / none / none | 0.62 | 0.62 | +0.97% | 2.86% | 1.00% | 20 | +0.04% | 0.13 | 1.42 |
| TPCH(2) | TPCH-Q3 | parquet / none / none | 0.88 | 0.87 | +0.57% | 2.17% | 1.74% | 20 | +0.29% | 1.15 | 0.92 |
| TPCH(2) | TPCH-Q12 | parquet / none / none | 0.53 | 0.53 | +0.27% | 4.58% | 5.78% | 20 | +0.46% | 1.47 | 0.16 |
| TPCH(2) | TPCH-Q17 | parquet / none / none | 0.72 | 0.72 | +0.15% | 3.64% | 5.55% | 20 | +0.21% | 0.86 | 0.10 |
| TPCH(2) | TPCH-Q21 | parquet / none / none | 2.05 | 2.05 | +0.21% | 1.99% | 2.37% | 20 | +0.01% | 0.25 | 0.30 |
| TPCH(2) | TPCH-Q5 | parquet / none / none | 1.28 | 1.27 | +0.24% | 1.61% | 1.80% | 20 | -0.02% | -0.57 | 0.44 |
| TPCH(2) | TPCH-Q13 | parquet / none / none | 1.27 | 1.27 | -0.34% | 1.69% | 1.83% | 20 | -0.20% | -1.65 | -0.61 |
| TPCH(2) | TPCH-Q7 | parquet / none / none | 1.72 | 1.73 | -0.55% | 2.40% | 1.69% | 20 | -0.03% | -0.42 | -0.83 |
| TPCH(2) | TPCH-Q8 | parquet / none / none | 1.27 | 1.28 | -0.68% | 3.10% | 3.89% | 20 | -0.06% | -0.54 | -0.62 |
| TPCH(2) | TPCH-Q6 | parquet / none / none | 0.36 | 0.36 | -0.84% | 0.79% | 3.51% | 20 | -0.07% | -0.36 | -1.04 |
| TPCH(2) | TPCH-Q2 | parquet / none / none | 0.65 | 0.65 | -1.17% | 4.76% | 5.99% | 20 | -0.05% | -0.25 | -0.69 |
| TPCH(2) | TPCH-Q9 | parquet / none / none | 1.59 | 1.62 | -2.01% | 1.45% | 5.12% | 20 | -0.16% | -1.24 | -1.69 |
| TPCH(2) | TPCH-Q20 | parquet / none / none | 0.68 | 0.69 | -1.73% | 4.35% | 4.43% | 20 | -0.49% | -1.74 | -1.25 |
| TPCH(2) | TPCH-Q22 | parquet / none / none | 0.38 | 0.40 | -2.89% | 7.42% | 6.39% | 20 | -0.21% | -0.66 | -1.34 |
| TPCH(2) | TPCH-Q16 | parquet / none / none | 0.59 | 0.62 | -4.01% | 6.33% | 5.83% | 20 | -4.72% | -1.39 | -2.13 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregator.cc
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/union-node.cc
M be/src/exprs/CMakeLists.txt
M be/src/exprs/agg-fn.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/case-expr.h
M be/src/exprs/compound-predicates.cc
M be/src/exprs/compound-predicates.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/literal.cc
M be/src/exprs/literal.h
M be/src/exprs/null-literal.cc
M be/src/exprs/null-literal.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/scalar-expr-ir.cc
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
A be/src/exprs/scalar-expr.inline.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
D be/src/exprs/slot-ref-ir.cc
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
R be/src/runtime/collection-value.cc
M be/src/runtime/collection-value.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tuple.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/util/tuple-row-compare.cc
M testdata/workloads/functional-query/queries/QueryTest/datastream-sender-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/disable-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
A testdata/workloads/tpcds-insert/queries/expr-insert.test
M tests/query_test/test_codegen.py
M tests/query_test/test_tpcds_queries.py
66 files changed, 841 insertions(+), 919 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12797/14
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 14
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12797
to look at the new patch set (#5).
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Based on initial draft patch by Pooja Nilangekar.
Codegen'd expressions can be executed in two ways - either by
being called directly from a fully codegend function, or from
interpreted code via a function pointer (previously
ScalarFnCall::scalar_fn_wrapper_).
This change moves the function pointer from ScalarFnCall to its
base class ScalarExpr, so the full expr tree can be codegen'd, not
just the ScalarFnCall subtrees. The key refactoring and improvements
are:
* ScalarExpr::Get*Val() switches between interpreted and the codegen'd
function pointer code paths in an inline function, avoiding a
virtual function call to ScalarFnCal::Get*Val().
* Boilerplate logic is moved to ScalarExpr::GetCodegendComputeFn(),
which calls a virtual function GetCodegenComputeFnImpl().
* ScalarFnCall's logic for deciding whether to interpret or codegen is
better abstracted and exposed to ScalarExpr as IsInterpretable()
and ShouldCodegen() methods.
* The ScalarExpr::codegend_compute_fn_ function pointer is only
populated for expressions that are "codegen entry points". These
include the roots of expr trees and non-root expressions
where the parent expression calls Get*Val() from the
pseudo-codegend GetCodegendComputeFnWrapper().
* ScalarFnCall is always initialised for interpreted execution.
Otherwise the function pointer is needed for non-root expressions,
e.g. to support ScalarExprEvaluator::GetConstantVal().
* Latent bugs/gaps for codegen of CollectionVal are fixed. CollectionVal
is modified to use the StringVal memory layout to allow code sharing
with StringVal. These fixes allowed simplification of
IsNotEmptyPredicate codegen (from IMPALA-7657).
IMPALA-7331 (CHAR codegen support functions) is also fixed because
it was simpler to enable CHAR codegen within ScalarExpr than to carry
forward the exiting CHAR workarounds from ScalarFnCall. The CHAR-specific
codegen support required in the scalar expr subsystem is very limited.
StringVal intermediates are used everywhere. Only SlotRef actually
operates on the different tuple layout, and the required codegen support
for SlotRef already exists for UDA intermediates anyway.
Testing:
* Ran exhaustive tests.
Perf:
* Ran a basic insert benchmark, which went from 10.1s to 7.6s
create table foo stored as parquet as
select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end
from tpch30_parquet.lineitem;
* Added perf regression test to tpcds-insert, similar to the manual
benchmark.
* Ran single-node TPC-H with large and small scale factors, to estimate
impact on execution perf and query startup time, respectively.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 6.84 | -0.18% | 4.49 | -0.31% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.58 | 2.47 | +4.18% | 1.29% | 0.88% | 5 | +4.12% | 2.31 | 5.81 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 4.81 | 4.61 | +4.33% | 2.18% | 2.15% | 5 | +3.91% | 1.73 | 3.09 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 26.45 | 26.16 | +1.09% | 0.37% | 0.50% | 5 | +1.36% | 2.02 | 3.94 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 15.92 | 15.75 | +1.09% | 2.87% | 1.65% | 5 | +0.88% | 0.29 | 0.73 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.38 | 2.35 | +1.12% | 1.64% | 1.11% | 5 | +0.80% | 1.15 | 1.26 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.13% | 7.68% | 5.37% | 5 | -0.34% | -0.29 | 0.27 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 18.10 | 18.02 | +0.42% | 2.70% | 0.56% | 5 | +0.28% | 0.29 | 0.34 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 4.72 | 4.72 | -0.04% | 1.20% | 1.65% | 5 | +0.05% | 0.00 | -0.04 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.92 | 3.93 | -0.26% | 1.08% | 2.36% | 5 | +0.20% | 0.58 | -0.23 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.27 | 1.27 | -0.28% | 0.22% | 0.88% | 5 | +0.09% | 0.29 | -0.68 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.64 | 2.65 | -0.45% | 1.65% | 0.65% | 5 | -0.24% | -0.58 | -0.57 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 3.10 | 3.13 | -0.76% | 1.47% | 1.12% | 5 | -0.21% | -0.29 | -0.93 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 1.20 | 1.21 | -0.80% | 2.26% | 2.47% | 5 | -0.82% | -1.15 | -0.53 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 1.97 | 1.99 | -1.37% | 1.84% | 3.21% | 5 | -0.47% | -0.58 | -0.83 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 11.53 | 11.63 | -0.91% | 0.46% | 0.49% | 5 | -0.95% | -2.02 | -3.08 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 5.13 | 5.21 | -1.51% | 2.24% | 4.05% | 5 | -0.94% | -0.58 | -0.73 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 3.61 | 3.66 | -1.40% | 0.66% | 0.79% | 5 | -1.33% | -1.73 | -3.05 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 19.42 | 19.71 | -1.52% | 1.34% | 1.39% | 5 | -1.22% | -1.44 | -1.76 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 5.08 | 5.15 | -1.49% | 1.34% | 0.73% | 5 | -1.35% | -1.44 | -2.20 |
| TPCH(30) | TPCH-Q15 | parquet / none / none | 3.42 | 3.49 | -1.92% | 0.93% | 1.47% | 5 | -1.53% | -1.15 | -2.49 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.15 | 1.19 | -3.17% | 2.27% | 1.95% | 5 | -4.21% | -1.15 | -2.41 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 9.26 | 9.63 | -3.85% | 0.62% | 0.59% | 5 | -3.78% | -2.31 | -10.25 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
Cluster Name: UNKNOWN
Lab Run Info: UNKNOWN
Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE ()
Baseline Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE (2019-03-19)
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(2) | parquet / none / none | 0.90 | -0.08% | 0.80 | -0.05% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(2) | TPCH-Q18 | parquet / none / none | 1.22 | 1.19 | +1.93% | 3.81% | 4.46% | 20 | +3.34% | 1.62 | 1.46 |
| TPCH(2) | TPCH-Q10 | parquet / none / none | 0.74 | 0.73 | +1.97% | 3.36% | 2.94% | 20 | +0.97% | 1.88 | 1.95 |
| TPCH(2) | TPCH-Q11 | parquet / none / none | 0.49 | 0.48 | +1.91% | 6.19% | 4.64% | 20 | +0.25% | 0.95 | 1.09 |
| TPCH(2) | TPCH-Q4 | parquet / none / none | 0.43 | 0.43 | +1.99% | 6.26% | 5.86% | 20 | +0.15% | 0.92 | 1.03 |
| TPCH(2) | TPCH-Q15 | parquet / none / none | 0.50 | 0.49 | +1.82% | 7.32% | 6.35% | 20 | +0.26% | 1.01 | 0.83 |
| TPCH(2) | TPCH-Q1 | parquet / none / none | 0.98 | 0.97 | +0.79% | 4.64% | 2.73% | 20 | +0.36% | 0.77 | 0.65 |
| TPCH(2) | TPCH-Q19 | parquet / none / none | 0.83 | 0.83 | +0.65% | 3.33% | 2.80% | 20 | +0.44% | 2.18 | 0.67 |
| TPCH(2) | TPCH-Q14 | parquet / none / none | 0.62 | 0.62 | +0.97% | 2.86% | 1.00% | 20 | +0.04% | 0.13 | 1.42 |
| TPCH(2) | TPCH-Q3 | parquet / none / none | 0.88 | 0.87 | +0.57% | 2.17% | 1.74% | 20 | +0.29% | 1.15 | 0.92 |
| TPCH(2) | TPCH-Q12 | parquet / none / none | 0.53 | 0.53 | +0.27% | 4.58% | 5.78% | 20 | +0.46% | 1.47 | 0.16 |
| TPCH(2) | TPCH-Q17 | parquet / none / none | 0.72 | 0.72 | +0.15% | 3.64% | 5.55% | 20 | +0.21% | 0.86 | 0.10 |
| TPCH(2) | TPCH-Q21 | parquet / none / none | 2.05 | 2.05 | +0.21% | 1.99% | 2.37% | 20 | +0.01% | 0.25 | 0.30 |
| TPCH(2) | TPCH-Q5 | parquet / none / none | 1.28 | 1.27 | +0.24% | 1.61% | 1.80% | 20 | -0.02% | -0.57 | 0.44 |
| TPCH(2) | TPCH-Q13 | parquet / none / none | 1.27 | 1.27 | -0.34% | 1.69% | 1.83% | 20 | -0.20% | -1.65 | -0.61 |
| TPCH(2) | TPCH-Q7 | parquet / none / none | 1.72 | 1.73 | -0.55% | 2.40% | 1.69% | 20 | -0.03% | -0.42 | -0.83 |
| TPCH(2) | TPCH-Q8 | parquet / none / none | 1.27 | 1.28 | -0.68% | 3.10% | 3.89% | 20 | -0.06% | -0.54 | -0.62 |
| TPCH(2) | TPCH-Q6 | parquet / none / none | 0.36 | 0.36 | -0.84% | 0.79% | 3.51% | 20 | -0.07% | -0.36 | -1.04 |
| TPCH(2) | TPCH-Q2 | parquet / none / none | 0.65 | 0.65 | -1.17% | 4.76% | 5.99% | 20 | -0.05% | -0.25 | -0.69 |
| TPCH(2) | TPCH-Q9 | parquet / none / none | 1.59 | 1.62 | -2.01% | 1.45% | 5.12% | 20 | -0.16% | -1.24 | -1.69 |
| TPCH(2) | TPCH-Q20 | parquet / none / none | 0.68 | 0.69 | -1.73% | 4.35% | 4.43% | 20 | -0.49% | -1.74 | -1.25 |
| TPCH(2) | TPCH-Q22 | parquet / none / none | 0.38 | 0.40 | -2.89% | 7.42% | 6.39% | 20 | -0.21% | -0.66 | -1.34 |
| TPCH(2) | TPCH-Q16 | parquet / none / none | 0.59 | 0.62 | -4.01% | 6.33% | 5.83% | 20 | -4.72% | -1.39 | -2.13 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregator.cc
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/union-node.cc
M be/src/exprs/CMakeLists.txt
M be/src/exprs/agg-fn.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/case-expr.h
M be/src/exprs/compound-predicates.cc
M be/src/exprs/compound-predicates.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/literal.cc
M be/src/exprs/literal.h
M be/src/exprs/null-literal.cc
M be/src/exprs/null-literal.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/scalar-expr-ir.cc
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
A be/src/exprs/scalar-expr.inline.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
D be/src/exprs/slot-ref-ir.cc
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
R be/src/runtime/collection-value.cc
M be/src/runtime/collection-value.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tuple.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/util/tuple-row-compare.cc
M testdata/workloads/functional-query/queries/QueryTest/datastream-sender-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/disable-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
A testdata/workloads/tpcds-insert/queries/expr-insert.test
M tests/query_test/test_codegen.py
M tests/query_test/test_tpcds_queries.py
66 files changed, 905 insertions(+), 805 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12797/5
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 6:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG@41
PS6, Line 41: IMPALA-7331 (CHAR codegen support functions) is also fixed becaus
> Shouldn't this speed up CHAR(N) manipulation? It would be nice to see some
I added a toy manual benchmark below that shows a significant improvement. Do you think it's worth adding something to targeted-perf? It feels a bit contrived unless we have a perf dataset with CHAR columns.
http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG@43
PS6, Line 43: The CHAR-specific
> nit: wrap at 72 chars
Done
http://gerrit.cloudera.org:8080/#/c/12797/6//COMMIT_MSG@53
PS6, Line 53: * Ran a basic insert benchmark, which went from 10.1s to 7.6s
: create table foo stored as parquet as
: select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end
: from tpch30_parquet.lineitem;
: * Added perf regression test to tpcds-insert, similar to the manual
: benchmark.
> Is there a specific reason for using INSERT for benchmarking?
INSERT was one of the main motivators for this work, e.g. see IMPALA-4358.
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/codegen/codegen-anyval.h
File be/src/codegen/codegen-anyval.h:
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/codegen/codegen-anyval.h@52
PS6, Line 52: /// TYPE_STRING,TYPE_VARCHAR,TYPE_CHAR,TYPE_FIXED_UDA_INTERMEDIATE/StringVal: { i64, i8* }
> Collections could be also mentioned here.
Done
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-expr.h
File be/src/exprs/scalar-expr.h:
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-expr.h@85
PS6, Line 85: One is implemented for each
: /// possible return type it supports (e.g. GetBooleanVal(), GetStringVal(), etc). The
: /// return type is a subclass of AnyVal (e.g. StringVal). One or more of these compute
: /// functions must be overridden by subclasses of ScalarExpr.
> In the new version the *Interpreted() functions have to be overriden by sub
Done
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-expr.h@265
PS6, Line 265: Interpretd
> typo
Done
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.h
File be/src/exprs/scalar-fn-call.h:
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.h@47
PS6, Line 47: GetCodegendComputeFn
> "Impl" is missing.
Done
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.h@48
PS6, Line 48: e
> nit: long line could be fixed if this comment is touched
Done
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.h@49
PS6, Line 49: If codegen is enabled, ScalarFnCall's Get*Val() compute
: /// functions are wrappers around this codegen'd function.
> This sentence would be more relevant in scalar-expr.h
I think the updated comment in scalar-expr.h covers this, so just deleted this sentence.
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/6/be/src/exprs/scalar-fn-call.cc@483
PS6, Line 483: // TODO: macroify this?
> +1 for this TODO, I think that it would be better to do this with macros bo
Done
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 06 Apr 2019 00:41:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 10:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.h
File be/src/exprs/scalar-expr.h:
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.h@87
PS10, Line 87: Get*Val() dispatches to either
: /// a codegen'd function pointer or to an interpreted implementation Get*ValInterpreted()
: /// These interpreted functions must be overridden by subclasses of ScalarExpr for every
: /// type that they may return.
: ///
: /// The two main usage patterns for ScalarExpr are:
: /// * The codegen'd expressions are called from other codegen'd functions, e.g. from a
: /// codegen'd join implementation
: /// * Get*Val() is called on the root of each expression subtree by interpreted code.
: /// We can optimize for the second usage pattern by filling in the codegen'd function
: /// pointer (codegend_compute_fn_) in root of each ScalarExpr tree. Individual callsites
: /// can disable this optimisation if it's not needed. Expr subtrees can be evaluated
: /// (e.g. by ScalarExprEvaluator::GetConstValue()) but may fail back to a slower
: /// interpreted implementation.
These sets of comments seem to fit better after line 107-114. Reading this comment, it's a bit hard to understand what's being talked about without knowing what codegen is. It will be clearer to first do an introduction of codegen and its relationship with ScalarExpr and various functions before pointing out that Get*Val() is a wrapper which dispatches to codegen'd function by default but can also fall back to the interpreted version if it's not codegen'd.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.h@170
PS10, Line 170: bool is_codegen_entry_point,
I wonder how much measurable benefit we get from this. IMHO, this can be an optimization as a follow-up patch. Doesn't seem 100% necessary functionally for this patch.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.h@288
PS10, Line 288: If 'is_entry_point' is true, this indicates that Get*Val()
: /// may be called directly from interpreted code and that we should generate an entry
: /// point into the codegen'd code.
Please see comments in scalar-expr.cc. 'is_entry_point' may not need to be exposed.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.h@349
PS10, Line 349: /// The vast majority of exprs support interpretation, so default to true.
May want to point out that expressions which aren't interpretable should override this function etc.
Will also help to explain more elaborately why an expression is not interpretable. IMHO, this concept is a bit more on the side of implementation details and exposing this interface is a bit unfortunate as it inter-mingles the already complicated concept of ScalarExpr with codegen.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.h@375
PS10, Line 375: codegend_compute_fn_ = nullptr;
Please document that this is left as null if this scalar expression is not an "entry point".
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.cc
File be/src/exprs/scalar-expr.cc:
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.cc@284
PS10, Line 284: bool is_entry_point
May be I didn't follow all the details correctly but it seems to me that all these plumbing for passing this flag around is only used in GetCodegendComputeFn().
In theory, only the top level expression (i.e. the root of the scalar expression tree) may require exposing itself with a function pointer after codegen (i.e. it's an entry point).
So, why cannot 'is_entry_point' be a property of a ScalarExpr itself ? In other words, a simple implementation is to set 'is_entry_point_' to true for the root of all ScalarExpr and leave it as false for any sub-expressions in the tree. In this way, we don't need to plumb this flag around. More importantly, this seems to be implementation specific details / optimization that we probably should try not to expose it in the interface of Init() if possible.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.cc@409
PS10, Line 409: llvm::Function* static_getval_fn = GetStaticGetValWrapper(type(), codegen);
if (static_getval_fn == nullptr) {
return ....
}
We only get a DCHECK in debug build. Failing to codegen shouldn't be fatal.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.inline.h
File be/src/exprs/scalar-expr.inline.h:
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.inline.h@25
PS10, Line 25: #define SCALAR_EXPR_GET_VAL(val_type, type_validation)
Please document the purpose of this macro and its arguments. Also please also point out that if the expression is not codegen'd, Get*ValInterpreted() will be called instead.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-expr.inline.h@27
PS10, Line 27: ScalarExpr::Get##val_type(
I am not a big fan of this type of macro code generation as it makes finding where function is defined a bit hard some time.
May help to add a comment that the following macro defines
- ScalarExpr::GetIntVal()
- ScalarExpr::Get...
so people doing a grep will at least be able to find this file.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-fn-call.h
File be/src/exprs/scalar-fn-call.h:
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-fn-call.h@87
PS10, Line 87: virtual bool IsInterpretable() const override;
Warrants a comment on why this is overridden here.
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-fn-call.h@112
PS10, Line 112: /// functions. Not set for IR UDFs.
Please consider keeping the original part of the comment about that this is only invoked when codegen'd path is not used somehow ?
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:
http://gerrit.cloudera.org:8080/#/c/12797/10/be/src/exprs/scalar-fn-call.cc@484
PS10, Line 484: #define GET_VAL_INTERPRETED(val_type, type_validation) \
Similar comments as scalar-expr.inline.h: May help to add a comment to say this macro generates:
- ScalarFnCall::GetBooleanInterpreted(0
- ScalarFnCall::Get...
Makes searching in the code slightly easier.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 10
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 22 Apr 2019 23:21:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12797 )
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
Patch Set 13:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3188/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 13
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 11 May 2019 01:41:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Thomas Marshall, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12797
to look at the new patch set (#9).
Change subject: IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
......................................................................
IMPALA-4356,IMPALA-7331: codegen all ScalarExprs
Based on initial draft patch by Pooja Nilangekar.
Codegen'd expressions can be executed in two ways - either by
being called directly from a fully codegend function, or from
interpreted code via a function pointer (previously
ScalarFnCall::scalar_fn_wrapper_).
This change moves the function pointer from ScalarFnCall to its
base class ScalarExpr, so the full expr tree can be codegen'd, not
just the ScalarFnCall subtrees. The key refactoring and improvements
are:
* ScalarExpr::Get*Val() switches between interpreted and the codegen'd
function pointer code paths in an inline function, avoiding a
virtual function call to ScalarFnCal::Get*Val().
* Boilerplate logic is moved to ScalarExpr::GetCodegendComputeFn(),
which calls a virtual function GetCodegenComputeFnImpl().
* ScalarFnCall's logic for deciding whether to interpret or codegen is
better abstracted and exposed to ScalarExpr as IsInterpretable()
and ShouldCodegen() methods.
* The ScalarExpr::codegend_compute_fn_ function pointer is only
populated for expressions that are "codegen entry points". These
include the roots of expr trees and non-root expressions
where the parent expression calls Get*Val() from the
pseudo-codegend GetCodegendComputeFnWrapper().
* ScalarFnCall is always initialised for interpreted execution.
Otherwise the function pointer is needed for non-root expressions,
e.g. to support ScalarExprEvaluator::GetConstantVal().
* Latent bugs/gaps for codegen of CollectionVal are fixed. CollectionVal
is modified to use the StringVal memory layout to allow code sharing
with StringVal. These fixes allowed simplification of
IsNotEmptyPredicate codegen (from IMPALA-7657).
IMPALA-7331 (CHAR codegen support functions) is also fixed because
it was simpler to enable CHAR codegen within ScalarExpr than to carry
forward the exiting CHAR workarounds from ScalarFnCall. The
CHAR-specific codegen support required in the scalar expr subsystem is
very limited. StringVal intermediates are used everywhere. Only
SlotRef actually operates on the different tuple layout, and the
required codegen support for SlotRef already exists for UDA
intermediates anyway.
Testing:
* Ran exhaustive tests.
Perf:
* Ran a basic insert benchmark, which went from 10.1s to 7.6s
create table foo stored as parquet as
select case when l_orderkey % 2 = 0 then 'aaa' else 'bbb' end
from tpch30_parquet.lineitem;
* Ran a basic CHAR expr test:
set num_nodes=1;
set mt_dop=1;
select count(*) from lineitem
where cast(l_linestatus as CHAR(2)) = 'O ' and
cast(l_returnflag as CHAR(2)) = 'N '
The time spent in the scan went from 520ms to 220ms.
* Added perf regression test to tpcds-insert, similar to the manual
benchmark.
* Ran single-node TPC-H with large and small scale factors, to estimate
impact on execution perf and query startup time, respectively.
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 6.84 | -0.18% | 4.49 | -0.31% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
| TPCH(30) | TPCH-Q20 | parquet / none / none | 2.58 | 2.47 | +4.18% | 1.29% | 0.88% | 5 | +4.12% | 2.31 | 5.81 |
| TPCH(30) | TPCH-Q17 | parquet / none / none | 4.81 | 4.61 | +4.33% | 2.18% | 2.15% | 5 | +3.91% | 1.73 | 3.09 |
| TPCH(30) | TPCH-Q21 | parquet / none / none | 26.45 | 26.16 | +1.09% | 0.37% | 0.50% | 5 | +1.36% | 2.02 | 3.94 |
| TPCH(30) | TPCH-Q9 | parquet / none / none | 15.92 | 15.75 | +1.09% | 2.87% | 1.65% | 5 | +0.88% | 0.29 | 0.73 |
| TPCH(30) | TPCH-Q12 | parquet / none / none | 2.38 | 2.35 | +1.12% | 1.64% | 1.11% | 5 | +0.80% | 1.15 | 1.26 |
| TPCH(30) | TPCH-Q14 | parquet / none / none | 2.94 | 2.91 | +1.13% | 7.68% | 5.37% | 5 | -0.34% | -0.29 | 0.27 |
| TPCH(30) | TPCH-Q18 | parquet / none / none | 18.10 | 18.02 | +0.42% | 2.70% | 0.56% | 5 | +0.28% | 0.29 | 0.34 |
| TPCH(30) | TPCH-Q8 | parquet / none / none | 4.72 | 4.72 | -0.04% | 1.20% | 1.65% | 5 | +0.05% | 0.00 | -0.04 |
| TPCH(30) | TPCH-Q19 | parquet / none / none | 3.92 | 3.93 | -0.26% | 1.08% | 2.36% | 5 | +0.20% | 0.58 | -0.23 |
| TPCH(30) | TPCH-Q6 | parquet / none / none | 1.27 | 1.27 | -0.28% | 0.22% | 0.88% | 5 | +0.09% | 0.29 | -0.68 |
| TPCH(30) | TPCH-Q16 | parquet / none / none | 2.64 | 2.65 | -0.45% | 1.65% | 0.65% | 5 | -0.24% | -0.58 | -0.57 |
| TPCH(30) | TPCH-Q22 | parquet / none / none | 3.10 | 3.13 | -0.76% | 1.47% | 1.12% | 5 | -0.21% | -0.29 | -0.93 |
| TPCH(30) | TPCH-Q2 | parquet / none / none | 1.20 | 1.21 | -0.80% | 2.26% | 2.47% | 5 | -0.82% | -1.15 | -0.53 |
| TPCH(30) | TPCH-Q4 | parquet / none / none | 1.97 | 1.99 | -1.37% | 1.84% | 3.21% | 5 | -0.47% | -0.58 | -0.83 |
| TPCH(30) | TPCH-Q13 | parquet / none / none | 11.53 | 11.63 | -0.91% | 0.46% | 0.49% | 5 | -0.95% | -2.02 | -3.08 |
| TPCH(30) | TPCH-Q10 | parquet / none / none | 5.13 | 5.21 | -1.51% | 2.24% | 4.05% | 5 | -0.94% | -0.58 | -0.73 |
| TPCH(30) | TPCH-Q5 | parquet / none / none | 3.61 | 3.66 | -1.40% | 0.66% | 0.79% | 5 | -1.33% | -1.73 | -3.05 |
| TPCH(30) | TPCH-Q7 | parquet / none / none | 19.42 | 19.71 | -1.52% | 1.34% | 1.39% | 5 | -1.22% | -1.44 | -1.76 |
| TPCH(30) | TPCH-Q3 | parquet / none / none | 5.08 | 5.15 | -1.49% | 1.34% | 0.73% | 5 | -1.35% | -1.44 | -2.20 |
| TPCH(30) | TPCH-Q15 | parquet / none / none | 3.42 | 3.49 | -1.92% | 0.93% | 1.47% | 5 | -1.53% | -1.15 | -2.49 |
| TPCH(30) | TPCH-Q11 | parquet / none / none | 1.15 | 1.19 | -3.17% | 2.27% | 1.95% | 5 | -4.21% | -1.15 | -2.41 |
| TPCH(30) | TPCH-Q1 | parquet / none / none | 9.26 | 9.63 | -3.85% | 0.62% | 0.59% | 5 | -3.78% | -2.31 | -10.25 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+--------+
Cluster Name: UNKNOWN
Lab Run Info: UNKNOWN
Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE ()
Baseline Impala Version: impalad version 3.2.0-SNAPSHOT RELEASE (2019-03-19)
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(2) | parquet / none / none | 0.90 | -0.08% | 0.80 | -0.05% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Workload | Query | File Format | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH(2) | TPCH-Q18 | parquet / none / none | 1.22 | 1.19 | +1.93% | 3.81% | 4.46% | 20 | +3.34% | 1.62 | 1.46 |
| TPCH(2) | TPCH-Q10 | parquet / none / none | 0.74 | 0.73 | +1.97% | 3.36% | 2.94% | 20 | +0.97% | 1.88 | 1.95 |
| TPCH(2) | TPCH-Q11 | parquet / none / none | 0.49 | 0.48 | +1.91% | 6.19% | 4.64% | 20 | +0.25% | 0.95 | 1.09 |
| TPCH(2) | TPCH-Q4 | parquet / none / none | 0.43 | 0.43 | +1.99% | 6.26% | 5.86% | 20 | +0.15% | 0.92 | 1.03 |
| TPCH(2) | TPCH-Q15 | parquet / none / none | 0.50 | 0.49 | +1.82% | 7.32% | 6.35% | 20 | +0.26% | 1.01 | 0.83 |
| TPCH(2) | TPCH-Q1 | parquet / none / none | 0.98 | 0.97 | +0.79% | 4.64% | 2.73% | 20 | +0.36% | 0.77 | 0.65 |
| TPCH(2) | TPCH-Q19 | parquet / none / none | 0.83 | 0.83 | +0.65% | 3.33% | 2.80% | 20 | +0.44% | 2.18 | 0.67 |
| TPCH(2) | TPCH-Q14 | parquet / none / none | 0.62 | 0.62 | +0.97% | 2.86% | 1.00% | 20 | +0.04% | 0.13 | 1.42 |
| TPCH(2) | TPCH-Q3 | parquet / none / none | 0.88 | 0.87 | +0.57% | 2.17% | 1.74% | 20 | +0.29% | 1.15 | 0.92 |
| TPCH(2) | TPCH-Q12 | parquet / none / none | 0.53 | 0.53 | +0.27% | 4.58% | 5.78% | 20 | +0.46% | 1.47 | 0.16 |
| TPCH(2) | TPCH-Q17 | parquet / none / none | 0.72 | 0.72 | +0.15% | 3.64% | 5.55% | 20 | +0.21% | 0.86 | 0.10 |
| TPCH(2) | TPCH-Q21 | parquet / none / none | 2.05 | 2.05 | +0.21% | 1.99% | 2.37% | 20 | +0.01% | 0.25 | 0.30 |
| TPCH(2) | TPCH-Q5 | parquet / none / none | 1.28 | 1.27 | +0.24% | 1.61% | 1.80% | 20 | -0.02% | -0.57 | 0.44 |
| TPCH(2) | TPCH-Q13 | parquet / none / none | 1.27 | 1.27 | -0.34% | 1.69% | 1.83% | 20 | -0.20% | -1.65 | -0.61 |
| TPCH(2) | TPCH-Q7 | parquet / none / none | 1.72 | 1.73 | -0.55% | 2.40% | 1.69% | 20 | -0.03% | -0.42 | -0.83 |
| TPCH(2) | TPCH-Q8 | parquet / none / none | 1.27 | 1.28 | -0.68% | 3.10% | 3.89% | 20 | -0.06% | -0.54 | -0.62 |
| TPCH(2) | TPCH-Q6 | parquet / none / none | 0.36 | 0.36 | -0.84% | 0.79% | 3.51% | 20 | -0.07% | -0.36 | -1.04 |
| TPCH(2) | TPCH-Q2 | parquet / none / none | 0.65 | 0.65 | -1.17% | 4.76% | 5.99% | 20 | -0.05% | -0.25 | -0.69 |
| TPCH(2) | TPCH-Q9 | parquet / none / none | 1.59 | 1.62 | -2.01% | 1.45% | 5.12% | 20 | -0.16% | -1.24 | -1.69 |
| TPCH(2) | TPCH-Q20 | parquet / none / none | 0.68 | 0.69 | -1.73% | 4.35% | 4.43% | 20 | -0.49% | -1.74 | -1.25 |
| TPCH(2) | TPCH-Q22 | parquet / none / none | 0.38 | 0.40 | -2.89% | 7.42% | 6.39% | 20 | -0.21% | -0.66 | -1.34 |
| TPCH(2) | TPCH-Q16 | parquet / none / none | 0.59 | 0.62 | -4.01% | 6.33% | 5.83% | 20 | -4.72% | -1.39 | -2.13 |
+----------+----------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
---
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/codegen-anyval.h
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/aggregator.cc
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/grouping-aggregator.cc
M be/src/exec/hash-table-test.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/union-node.cc
M be/src/exprs/CMakeLists.txt
M be/src/exprs/agg-fn.cc
M be/src/exprs/case-expr.cc
M be/src/exprs/case-expr.h
M be/src/exprs/compound-predicates.cc
M be/src/exprs/compound-predicates.h
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/hive-udf-call.cc
M be/src/exprs/hive-udf-call.h
M be/src/exprs/is-not-empty-predicate.cc
M be/src/exprs/is-not-empty-predicate.h
M be/src/exprs/kudu-partition-expr.cc
M be/src/exprs/kudu-partition-expr.h
M be/src/exprs/literal.cc
M be/src/exprs/literal.h
M be/src/exprs/null-literal.cc
M be/src/exprs/null-literal.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/scalar-expr-ir.cc
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
A be/src/exprs/scalar-expr.inline.h
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
D be/src/exprs/slot-ref-ir.cc
M be/src/exprs/slot-ref.cc
M be/src/exprs/slot-ref.h
M be/src/exprs/tuple-is-null-predicate.cc
M be/src/exprs/tuple-is-null-predicate.h
M be/src/exprs/valid-tuple-id.cc
M be/src/exprs/valid-tuple-id.h
M be/src/runtime/CMakeLists.txt
R be/src/runtime/collection-value.cc
M be/src/runtime/collection-value.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tuple.cc
M be/src/service/fe-support.cc
M be/src/udf/udf-internal.h
M be/src/util/tuple-row-compare.cc
M testdata/workloads/functional-query/queries/QueryTest/datastream-sender-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/disable-codegen.test
M testdata/workloads/functional-query/queries/QueryTest/udf.test
A testdata/workloads/tpcds-insert/queries/expr-insert.test
M tests/query_test/test_codegen.py
M tests/query_test/test_tpcds_queries.py
66 files changed, 782 insertions(+), 889 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/97/12797/9
--
To view, visit http://gerrit.cloudera.org:8080/12797
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I839d7a3a2f5e1309c33a1f66013ef11628c5dc11
Gerrit-Change-Number: 12797
Gerrit-PatchSet: 9
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>