You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org> on 2017/12/04 17:14:35 UTC
[Impala-ASF-CR] IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
Zoltan Borok-Nagy has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8748
Change subject: IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
......................................................................
IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
When Lz4Compressor::MaxOutputLen returns 0, it
means that the input is too large to compress.
When invoked Lz4Compressor::ProcessBlock with
an input too large, it silently produced a bogus
result. This bogus result even decompresses
successfully, but not to the data that was
originally compressed.
After this commit, Lz4Compressor::ProcessBlock
will return error if it cannot compress the
input.
I also added a comment on Codec::MaxOutputLen()
that return value 0 means that the input is too
large.
I added some checks after the invocations of
MaxOutputLen() where the compressor can be
a Lz4Compressor.
I added an automated test case to
be/src/util/decompress-test.cc.
Change-Id: Ifb0bc4ed98c5d7b628b791aa90ead36347b9fbb8
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/runtime/row-batch.cc
M be/src/util/codec.h
M be/src/util/compress.cc
M be/src/util/decompress-test.cc
M be/src/util/runtime-profile.cc
M common/thrift/generate_error_codes.py
8 files changed, 35 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/8748/1
--
To view, visit http://gerrit.cloudera.org:8080/8748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifb0bc4ed98c5d7b628b791aa90ead36347b9fbb8
Gerrit-Change-Number: 8748
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8748 )
Change subject: IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/8748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb0bc4ed98c5d7b628b791aa90ead36347b9fbb8
Gerrit-Change-Number: 8748
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Dec 2017 21:42:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
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/8748 )
Change subject: IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
......................................................................
IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
When Lz4Compressor::MaxOutputLen returns 0, it
means that the input is too large to compress.
When invoked Lz4Compressor::ProcessBlock with
an input too large, it silently produced a bogus
result. This bogus result even decompresses
successfully, but not to the data that was
originally compressed.
After this commit, Lz4Compressor::ProcessBlock
will return error if it cannot compress the
input.
I also added a comment on Codec::MaxOutputLen()
that return value 0 means that the input is too
large.
I added some checks after the invocations of
MaxOutputLen() where the compressor can be
a Lz4Compressor.
I added an automated test case to
be/src/util/decompress-test.cc.
Change-Id: Ifb0bc4ed98c5d7b628b791aa90ead36347b9fbb8
Reviewed-on: http://gerrit.cloudera.org:8080/8748
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/runtime/row-batch.cc
M be/src/util/codec.h
M be/src/util/compress.cc
M be/src/util/decompress-test.cc
M be/src/util/runtime-profile.cc
M common/thrift/generate_error_codes.py
8 files changed, 39 insertions(+), 1 deletion(-)
Approvals:
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifb0bc4ed98c5d7b628b791aa90ead36347b9fbb8
Gerrit-Change-Number: 8748
Gerrit-PatchSet: 3
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
[Impala-ASF-CR] IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8748 )
Change subject: IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
......................................................................
Patch Set 1:
(2 comments)
Thanks!
http://gerrit.cloudera.org:8080/#/c/8748/1/be/src/util/decompress-test.cc
File be/src/util/decompress-test.cc:
http://gerrit.cloudera.org:8080/#/c/8748/1/be/src/util/decompress-test.cc@445
PS1, Line 445: TEST_F(DecompressorTest, LZ4Huge) {
> Can you add a comment referencing the JIRA number? Might be helpful to unde
Done
http://gerrit.cloudera.org:8080/#/c/8748/1/be/src/util/decompress-test.cc@459
PS1, Line 459: // Compression succeeds!
> Maybe mention what the expected behaviour is? This almost sounds like it *s
Oops, it shouldn't be here, I replaced this comment.
--
To view, visit http://gerrit.cloudera.org:8080/8748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb0bc4ed98c5d7b628b791aa90ead36347b9fbb8
Gerrit-Change-Number: 8748
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Dec 2017 17:58:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8748 )
Change subject: IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb0bc4ed98c5d7b628b791aa90ead36347b9fbb8
Gerrit-Change-Number: 8748
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Dec 2017 18:01:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8748 )
Change subject: IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
......................................................................
Patch Set 1: Code-Review+2
(2 comments)
LGTM aside from a couple of comments about comments
http://gerrit.cloudera.org:8080/#/c/8748/1/be/src/util/decompress-test.cc
File be/src/util/decompress-test.cc:
http://gerrit.cloudera.org:8080/#/c/8748/1/be/src/util/decompress-test.cc@445
PS1, Line 445: TEST_F(DecompressorTest, LZ4Huge) {
Can you add a comment referencing the JIRA number? Might be helpful to understand the motivation for the test.
http://gerrit.cloudera.org:8080/#/c/8748/1/be/src/util/decompress-test.cc@459
PS1, Line 459: // Compression succeeds!
Maybe mention what the expected behaviour is? This almost sounds like it *should* succeed.
--
To view, visit http://gerrit.cloudera.org:8080/8748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb0bc4ed98c5d7b628b791aa90ead36347b9fbb8
Gerrit-Change-Number: 8748
Gerrit-PatchSet: 1
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Dec 2017 17:32:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/8748 )
Change subject: IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
......................................................................
Patch Set 2:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1561/
--
To view, visit http://gerrit.cloudera.org:8080/8748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb0bc4ed98c5d7b628b791aa90ead36347b9fbb8
Gerrit-Change-Number: 8748
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Mon, 04 Dec 2017 18:02:13 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8748
to look at the new patch set (#2).
Change subject: IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
......................................................................
IMPALA-5987: LZ4 Codec silently produces bogus compressed data for large inputs
When Lz4Compressor::MaxOutputLen returns 0, it
means that the input is too large to compress.
When invoked Lz4Compressor::ProcessBlock with
an input too large, it silently produced a bogus
result. This bogus result even decompresses
successfully, but not to the data that was
originally compressed.
After this commit, Lz4Compressor::ProcessBlock
will return error if it cannot compress the
input.
I also added a comment on Codec::MaxOutputLen()
that return value 0 means that the input is too
large.
I added some checks after the invocations of
MaxOutputLen() where the compressor can be
a Lz4Compressor.
I added an automated test case to
be/src/util/decompress-test.cc.
Change-Id: Ifb0bc4ed98c5d7b628b791aa90ead36347b9fbb8
---
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-util.cc
M be/src/runtime/row-batch.cc
M be/src/util/codec.h
M be/src/util/compress.cc
M be/src/util/decompress-test.cc
M be/src/util/runtime-profile.cc
M common/thrift/generate_error_codes.py
8 files changed, 39 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/8748/2
--
To view, visit http://gerrit.cloudera.org:8080/8748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb0bc4ed98c5d7b628b791aa90ead36347b9fbb8
Gerrit-Change-Number: 8748
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>