You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Daniel Becker (Code Review)" <ge...@cloudera.org> on 2020/07/01 23:39:57 UTC

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed 

Daniel Becker has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16134


Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................

IMPALA-7923: DecimalValue should be marked as packed

IMPALA-7473 and IMPALA-9111 were symptoms of a more general problem that
DecimalValue is not guaranteed to be aligned by the Impala runtime but
the compiler assumes it is and under some circumstances, it will emit
code for aligned loads to value_ when value_ is an int128.

This commit marks DecimalValue as packed so that the compiler does not
assume any alignment.

TODO: Maybe benchmark if this introduces performance regressions, but it
shouldn't.

Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
---
M be/src/exec/hdfs-avro-scanner-test.cc
M be/src/exec/orc-column-readers.h
M be/src/runtime/decimal-value.h
M be/src/util/decimal-util.h
M be/src/util/dict-test.cc
5 files changed, 33 insertions(+), 15 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/16134/2
-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 4: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/6102/


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 4
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Jul 2020 22:04:15 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6107/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 5
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Jul 2020 08:52:13 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 4: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 4
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Jul 2020 16:57:28 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 5: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 5
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Jul 2020 20:23:01 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 3:

I did the test on parquet/none/none, scale factor 15. Copied the results to the Jira issue.
TPCH-Q1 became a regression.


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Jul 2020 10:20:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 3:

I run single node perf tests on TPCH with scale factor 2, copied the report to the Jira issue.


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Jul 2020 23:57:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 3:

I'm in the process of loading the tables for TPCH scale factor 15, but it seems to be very slow but let's see.


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Jul 2020 17:15:01 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 3:

I was a little scared since Q1 is a decimal-intensive query, but seems to be fine when I tried to repro:

Report Generated on 2020-07-07
Run Description: "6c8a3dfc339e43a8992af2ff3429ba5940a061ec vs 513c19bc0a750960b97f0d4cd14a9bdc8bbd2860"

Cluster Name: UNKNOWN
Lab Run Info: UNKNOWN
Impala Version:          impalad version 4.0.0-SNAPSHOT RELEASE ()
Baseline Impala Version: impalad version 4.0.0-SNAPSHOT RELEASE (2020-07-01)

+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format           | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(30) | parquet / none / none | 10.05   | +0.18%     | 10.05      | +0.18%         |
+----------+-----------------------+---------+------------+------------+----------------+

+----------+---------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+------+
| Workload | Query   | File Format           | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+---------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+------+
| TPCH(30) | TPCH-Q1 | parquet / none / none | 10.05  | 10.03       |   +0.18%   |   1.16%   |   0.64%        | 20    |   -0.00%       | -0.04   | 0.61 |
+----------+---------+-----------------------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+------+


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Jul 2020 16:57:14 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 3:

I think scale factor 2 might be too small to get meaningful results - the query runtime is probably dominated by the start-up time at that scale so might not capture differences in execution. Could you maybe rerun with SF >= 10? That's usually enough so that startup time is a smaller fraction.


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Jul 2020 18:52:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 3:

I took a quick pass and this seems fine in concept. I think we probably want to run tests with a release build before merging. It'd be good to do a TPC-H single node run as a sanity check. You could spot check with TPC-H Q1 since that's very decimal intensive.


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Jul 2020 00:07:54 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 4:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6102/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 4
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Jul 2020 16:57:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 5:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6109/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 5
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Jul 2020 15:09:13 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/6478/ : 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/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Jul 2020 00:08:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

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/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................

IMPALA-7923: DecimalValue should be marked as packed

IMPALA-7473 and IMPALA-9111 were symptoms of a more general problem that
DecimalValue is not guaranteed to be aligned by the Impala runtime but
the compiler assumes it is and under some circumstances, it will emit
code for aligned loads to value_ when value_ is an int128.

This commit marks DecimalValue as packed so that the compiler does not
assume any alignment.

TODO: Maybe benchmark if this introduces performance regressions, but it
shouldn't.

Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Reviewed-on: http://gerrit.cloudera.org:8080/16134
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/hdfs-avro-scanner-test.cc
M be/src/exec/orc-column-readers.h
M be/src/runtime/decimal-value.h
M be/src/runtime/decimal-value.inline.h
M be/src/util/decimal-util.h
M be/src/util/dict-test.cc
6 files changed, 35 insertions(+), 21 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 6
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

GOod point about fixing the crash. I think the risk of a performance regression is low.

http://gerrit.cloudera.org:8080/#/c/16134/3/be/src/exec/hdfs-avro-scanner-test.cc
File be/src/exec/hdfs-avro-scanner-test.cc:

http://gerrit.cloudera.org:8080/#/c/16134/3/be/src/exec/hdfs-avro-scanner-test.cc@519
PS3, Line 519:   memcpy(&data[1], &d16v_value2, 16);
> I wonder if it makes sense to keep and update the BIG_ENDIAN branch. AFAIK 
Yeah we should feel free to remove these. There was some effort to make stuff endian-neutral, I think cause it's just a "good practice", but that code isn't tested and will probably never be used, so no point maintaining it.



-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Jul 2020 16:38:20 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 3: Code-Review+1

(1 comment)

lgtm
I am also interested in the benchmark, but from my side it can be merged as it is even if the patch introduces regression, as it fixes a crash.

http://gerrit.cloudera.org:8080/#/c/16134/3/be/src/exec/hdfs-avro-scanner-test.cc
File be/src/exec/hdfs-avro-scanner-test.cc:

http://gerrit.cloudera.org:8080/#/c/16134/3/be/src/exec/hdfs-avro-scanner-test.cc@519
PS3, Line 519:   memcpy(&data[1], &d16v_value2, 16);
I wonder if it makes sense to keep and update the BIG_ENDIAN branch. AFAIK Impala is never compiled on BIG_ENDIAN architectures, end we have static_asserts to ensure this.



-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Jul 2020 14:22:09 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................

IMPALA-7923: DecimalValue should be marked as packed

IMPALA-7473 and IMPALA-9111 were symptoms of a more general problem that
DecimalValue is not guaranteed to be aligned by the Impala runtime but
the compiler assumes it is and under some circumstances, it will emit
code for aligned loads to value_ when value_ is an int128.

This commit marks DecimalValue as packed so that the compiler does not
assume any alignment.

TODO: Maybe benchmark if this introduces performance regressions, but it
shouldn't.

Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
---
M be/src/exec/hdfs-avro-scanner-test.cc
M be/src/exec/orc-column-readers.h
M be/src/runtime/decimal-value.h
M be/src/runtime/decimal-value.inline.h
M be/src/util/decimal-util.h
M be/src/util/dict-test.cc
6 files changed, 35 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/16134/3
-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 5: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/6107/


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 5
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Jul 2020 13:59:06 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 5: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 5
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 08 Jul 2020 08:52:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7923: DecimalValue should be marked as packed

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/16134 )

Change subject: IMPALA-7923: DecimalValue should be marked as packed
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/6479/ : 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/16134
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I55f936a4f4f4b5faf129a9265222e64fc486b8ed
Gerrit-Change-Number: 16134
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Jul 2020 00:10:12 +0000
Gerrit-HasComments: No