You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2017/07/21 03:11:35 UTC
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Henry Robinson has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7478
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Change allocation pattern for Codec objects in RowBatch to be
stack-allocated. Make c'tors and Init() methods of codec implementations
publicly visible in order to do so.
Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
---
M be/src/experiments/compression-test.cc
M be/src/runtime/row-batch.cc
M be/src/util/codec.cc
M be/src/util/codec.h
M be/src/util/compress.cc
M be/src/util/compress.h
M be/src/util/decompress-test.cc
M be/src/util/decompress.cc
M be/src/util/decompress.h
M be/src/util/runtime-profile.cc
10 files changed, 209 insertions(+), 208 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7478/1
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 3: Code-Review+1
Please do a quick run of clang-tidy before merging. Thanks !
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7478/1/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:
PS1, Line 119: else {
> This is not strictly needed given 'compressor_cleanup' above. Not sure if i
I think we should leave it out - decompressor.Close() is guaranteed to be called and leaving this in would call it twice which would be unexpected.
Line 119: } else {
> Is this needed now?
Done
http://gerrit.cloudera.org:8080/#/c/7478/1/be/src/util/codec.h
File be/src/util/codec.h:
PS1, Line 181: out_buffer_
> This isn't NULL-initialized now - not sure if it's necessary but it seems s
Done
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 3: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/913/
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/919/
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Michael Ho, Sailesh Mukil, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7478
to look at the new patch set (#4).
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Change allocation pattern for Codec objects in RowBatch to be
stack-allocated. Make c'tors and Init() methods of codec implementations
publicly visible in order to do so.
Fix bit-rotting bug in row-batch-serialize-benchmark that made it abort
on start up.
Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/experiments/compression-test.cc
M be/src/runtime/row-batch.cc
M be/src/util/codec.cc
M be/src/util/codec.h
M be/src/util/compress.cc
M be/src/util/compress.h
M be/src/util/decompress-test.cc
M be/src/util/decompress.cc
M be/src/util/decompress.h
M be/src/util/runtime-profile.cc
11 files changed, 227 insertions(+), 222 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7478/4
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/913/
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/920/
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has uploaded a new patch set (#3).
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Change allocation pattern for Codec objects in RowBatch to be
stack-allocated. Make c'tors and Init() methods of codec implementations
publicly visible in order to do so.
Fix bit-rotting bug in row-batch-serialize-benchmark that made it abort
on start up.
Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/experiments/compression-test.cc
M be/src/runtime/row-batch.cc
M be/src/util/codec.cc
M be/src/util/codec.h
M be/src/util/compress.cc
M be/src/util/compress.h
M be/src/util/decompress-test.cc
M be/src/util/decompress.cc
M be/src/util/decompress.h
M be/src/util/runtime-profile.cc
11 files changed, 222 insertions(+), 221 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7478/3
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7478/1/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:
PS1, Line 119: decompressor.Close();
This is not strictly needed given 'compressor_cleanup' above. Not sure if it's a good practice to leave it out though ?!
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
Sailesh Mukil has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 1:
To be consistent in the usage of this new API, we should make the RowBatchSerializeBaseline::Serialize() also use the Lz4Compressor and Lz4Decompressor.
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 3:
I have difficulty getting clang-tidy to run locally, so I'll let GVO take care of that.
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7478/2/be/src/util/compress.h
File be/src/util/compress.h:
Line 47: virtual Status Init() override;
> missing WARN_UNUSED_RESULT.
Done
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7478/1/be/src/runtime/row-batch.cc
File be/src/runtime/row-batch.cc:
Line 119: decompressor.Close();
Is this needed now?
http://gerrit.cloudera.org:8080/#/c/7478/1/be/src/util/codec.h
File be/src/util/codec.h:
PS1, Line 181: out_buffer_
This isn't NULL-initialized now - not sure if it's necessary but it seems safer.
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/923/
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 4: Code-Review+2
Fix a compilation error in experiments/compression-test and a couple missing 'override's.
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7478/2/be/src/util/compress.h
File be/src/util/compress.h:
Line 47: virtual Status Init() override;
missing WARN_UNUSED_RESULT.
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 3: Code-Review+2
Thanks for addressing my comments. It looks like everyone else is happy so I'll +2
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 4: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/919/
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Change allocation pattern for Codec objects in RowBatch to be
stack-allocated. Make c'tors and Init() methods of codec implementations
publicly visible in order to do so.
Fix bit-rotting bug in row-batch-serialize-benchmark that made it abort
on start up.
Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Reviewed-on: http://gerrit.cloudera.org:8080/7478
Reviewed-by: Henry Robinson <he...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/experiments/compression-test.cc
M be/src/runtime/row-batch.cc
M be/src/util/codec.cc
M be/src/util/codec.h
M be/src/util/compress.cc
M be/src/util/compress.h
M be/src/util/decompress-test.cc
M be/src/util/decompress.cc
M be/src/util/decompress.h
M be/src/util/runtime-profile.cc
11 files changed, 228 insertions(+), 221 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Henry Robinson: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 5: Code-Review+2
Fix a couple clang-tidy warnings. Last time's the charm.
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Michael Ho, Sailesh Mukil, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7478
to look at the new patch set (#5).
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Change allocation pattern for Codec objects in RowBatch to be
stack-allocated. Make c'tors and Init() methods of codec implementations
publicly visible in order to do so.
Fix bit-rotting bug in row-batch-serialize-benchmark that made it abort
on start up.
Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/experiments/compression-test.cc
M be/src/runtime/row-batch.cc
M be/src/util/codec.cc
M be/src/util/codec.h
M be/src/util/compress.cc
M be/src/util/compress.h
M be/src/util/decompress-test.cc
M be/src/util/decompress.cc
M be/src/util/decompress.h
M be/src/util/runtime-profile.cc
11 files changed, 228 insertions(+), 221 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7478/5
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
Patch Set 4:
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/920/
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has uploaded a new patch set (#2).
Change subject: IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
......................................................................
IMPALA-5532: Stack-allocate compressors in RowBatch (de)serialization
Change allocation pattern for Codec objects in RowBatch to be
stack-allocated. Make c'tors and Init() methods of codec implementations
publicly visible in order to do so.
Fix bit-rotting bug in row-batch-serialize-benchmark that made it abort
on start up.
Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/experiments/compression-test.cc
M be/src/runtime/row-batch.cc
M be/src/util/codec.cc
M be/src/util/codec.h
M be/src/util/compress.cc
M be/src/util/compress.h
M be/src/util/decompress-test.cc
M be/src/util/decompress.cc
M be/src/util/decompress.h
M be/src/util/runtime-profile.cc
11 files changed, 222 insertions(+), 221 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/7478/2
--
To view, visit http://gerrit.cloudera.org:8080/7478
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6641f4a08bd2711c4f4515ab29a6e5418cbd5f51
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>