Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201706301510.v5UFAkrm032075@ip-10-146-233-104.ec2.internal>
Date: Fri, 30 Jun 2017 15:10:45 +0000
From: "Tim Armstrong (Code Review)" <gerrit@cloudera.org>
To: impala-cr@cloudera.com, reviews@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>, Dan Hecht <dhecht@cloudera.com>
Reply-To: tarmstrong@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5560=3A_always_store_CHAR=28N=29_inline_in_tuple=0A?=
In-Reply-To: <gerrit.1498580701000.I9c0b823ccff6b0c37f5267c548d096c29b8caac3@gerrit.cloudera.org>
References: <gerrit.1498580701000.I9c0b823ccff6b0c37f5267c548d096c29b8caac3@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Fri, 30 Jun 2017 15:10:51 -0000

Tim Armstrong has uploaded a new patch set (#5).

Change subject: IMPALA-5560: always store CHAR(N) inline in tuple
......................................................................

IMPALA-5560: always store CHAR(N) inline in tuple

This is done to simplify the CHAR(N) logic. I believe this is overall an
improvement - any benefits of the out-of-line storage that motivated
this optimisation originally were outweighed by the added complexity.

This also avoids IMPALA-5559 (fe/be have different notions of var-len),
which will unblock IMPALA-3200.

Pros:
* Reduce the number of code paths and improve test coverage.
  (e.g. avoids IMPALA-5559: fe/be have different notions of var-len)
* Reduced memory to store non-NULL data (saves 12-byte StringValue)
* Fewer branches in code -> save CPU cycles.
* If CHAR(N) performance is important, reduced complexity makes it
  easier to implement codegen.

Cons:
* Requires N bytes to store a NULL value.
* May hurt cache locality (although this is speculative in my mind).

The change is mostly mechanical - I removed MAX_CHAR_INLINE_LENGTH
and then removed branches that depended on that.

Testing:
Ran exhaustive build.

Change-Id: I9c0b823ccff6b0c37f5267c548d096c29b8caac3
---
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-avro-scanner-test.cc
M be/src/exec/hdfs-parquet-table-writer.cc
M be/src/exec/hdfs-text-table-writer.cc
M be/src/exec/parquet-column-readers.cc
M be/src/exprs/agg-fn-evaluator.cc
M be/src/exprs/anyval-util.h
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/exprs/slot-ref.cc
M be/src/runtime/raw-value-ir.cc
M be/src/runtime/raw-value.cc
M be/src/runtime/raw-value.inline.h
M be/src/runtime/string-value.h
M be/src/runtime/string-value.inline.h
M be/src/runtime/types.h
M be/src/service/fe-support.cc
M be/src/service/hs2-util.cc
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M testdata/workloads/functional-query/queries/QueryTest/spilling.test
19 files changed, 68 insertions(+), 129 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9c0b823ccff6b0c37f5267c548d096c29b8caac3
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>