You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2017/06/22 16:45:39 UTC
[kudu-CR] Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Will Berkeley has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7262
Change subject: Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
......................................................................
Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
In a CFile block with n non-null values, it's permissible to
seek to the spot after all rows. However, a CHECK_LT in
RleIntBlockDecoder::SeekToPositionInBlock was enforcing seeks to
be < n. This patches switches the CHECK_LT to a DCHECK_LE,
making it consistent with the checks in other decoders. The
problem was not caught in testing due to an omission of RLE
encoding from cfile-test.cc's
TestCFileBothCacheTypes.TestNullInts. The test now has coverage
of RLE encoding and, with slow tests on, fails every time
without the change to the CHECK.
Additionally, TestCFileBothCacheTypes.TestNullFloats was missing
coverage for bit shuffle encoding. This was also fixed.
Finally, RleBitMapBlockDecoder::SeekToPositionInBlock was
missing any check on its 'pos' argument, so the checks from
RleIntBlockDecoder::SeekToPositionInBlock were added to that
method as well.
Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/rle_block.h
2 files changed, 13 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7262/1
--
To view, visit http://gerrit.cloudera.org:8080/7262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7262/2/src/kudu/cfile/rle_block.h
File src/kudu/cfile/rle_block.h:
PS2, Line 342: "
> I don't think there should be data corruption since we're just seeking in t
Thank you for the clarification!
--
To view, visit http://gerrit.cloudera.org:8080/7262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
......................................................................
Patch Set 2: Code-Review+1
lgtm modulo the style nits from alexey
--
To view, visit http://gerrit.cloudera.org:8080/7262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7262/2/src/kudu/cfile/rle_block.h
File src/kudu/cfile/rle_block.h:
PS2, Line 342: .
> nit: drop the dot :)
I don't think there should be data corruption since we're just seeking in the decoder, so it _should_ be harmless.
CopyNextValues() ensures that we don't actually copy any data past the end (L403)
--
To view, visit http://gerrit.cloudera.org:8080/7262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/7262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7262
to look at the new patch set (#2).
Change subject: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
......................................................................
KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
In a CFile block with n non-null values, it's permissible to
seek to the spot after all rows. However, a CHECK_LT in
RleIntBlockDecoder::SeekToPositionInBlock was enforcing seeks to
be < n. This patches switches the CHECK_LT to a DCHECK_LE,
making it consistent with the checks in other decoders. The
problem was not caught in testing due to an omission of RLE
encoding from cfile-test.cc's
TestCFileBothCacheTypes.TestNullInts. The test now has coverage
of RLE encoding and, with slow tests on, fails every time
without the change to the CHECK.
Additionally, TestCFileBothCacheTypes.TestNullFloats was missing
coverage for bit shuffle encoding. This was also fixed.
Finally, RleBitMapBlockDecoder::SeekToPositionInBlock was
missing any check on its 'pos' argument, so the checks from
RleIntBlockDecoder::SeekToPositionInBlock were added to that
method as well.
Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/rle_block.h
2 files changed, 13 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7262/2
--
To view, visit http://gerrit.cloudera.org:8080/7262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.
Change subject: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7262/2/src/kudu/cfile/rle_block.h
File src/kudu/cfile/rle_block.h:
PS2, Line 145: .
> nit: drop the dot
Done
PS2, Line 342: .
> nit: drop the dot :)
Done
PS2, Line 342: .
> I don't think there should be data corruption since we're just seeking in t
Thanks Andrew for clearing this up.
--
To view, visit http://gerrit.cloudera.org:8080/7262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged.
Change subject: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
......................................................................
KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
In a CFile block with n non-null values, it's permissible to
seek to the spot after all rows. However, a CHECK_LT in
RleIntBlockDecoder::SeekToPositionInBlock was enforcing seeks to
be < n. This patches switches the CHECK_LT to a DCHECK_LE,
making it consistent with the checks in other decoders. The
problem was not caught in testing due to an omission of RLE
encoding from cfile-test.cc's
TestCFileBothCacheTypes.TestNullInts. The test now has coverage
of RLE encoding and, with slow tests on, fails every time
without the change to the CHECK.
Additionally, TestCFileBothCacheTypes.TestNullFloats was missing
coverage for bit shuffle encoding. This was also fixed.
Finally, RleBitMapBlockDecoder::SeekToPositionInBlock was
missing any check on its 'pos' argument, so the checks from
RleIntBlockDecoder::SeekToPositionInBlock were added to that
method as well.
Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Reviewed-on: http://gerrit.cloudera.org:8080/7262
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/rle_block.h
2 files changed, 13 insertions(+), 3 deletions(-)
Approvals:
Andrew Wong: Looks good to me, but someone else must approve
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7262/2//COMMIT_MSG
Commit Message:
PS2, Line 9: non-null
> null?
Ah, nevermind. Misread as something along the lines of "in a nullable block".
--
To view, visit http://gerrit.cloudera.org:8080/7262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change.
Change subject: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
......................................................................
Patch Set 2: -Code-Review
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7262/2//COMMIT_MSG
Commit Message:
PS2, Line 9: non-null
null?
--
To view, visit http://gerrit.cloudera.org:8080/7262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Andrew Wong, Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7262
to look at the new patch set (#3).
Change subject: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
......................................................................
KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
In a CFile block with n non-null values, it's permissible to
seek to the spot after all rows. However, a CHECK_LT in
RleIntBlockDecoder::SeekToPositionInBlock was enforcing seeks to
be < n. This patches switches the CHECK_LT to a DCHECK_LE,
making it consistent with the checks in other decoders. The
problem was not caught in testing due to an omission of RLE
encoding from cfile-test.cc's
TestCFileBothCacheTypes.TestNullInts. The test now has coverage
of RLE encoding and, with slow tests on, fails every time
without the change to the CHECK.
Additionally, TestCFileBothCacheTypes.TestNullFloats was missing
coverage for bit shuffle encoding. This was also fixed.
Finally, RleBitMapBlockDecoder::SeekToPositionInBlock was
missing any check on its 'pos' argument, so the checks from
RleIntBlockDecoder::SeekToPositionInBlock were added to that
method as well.
Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/rle_block.h
2 files changed, 13 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7262/3
--
To view, visit http://gerrit.cloudera.org:8080/7262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Re: [kudu-CR] Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by William Berkeley <wd...@gmail.com>.
Sure. Please see KUDU-2049 <https://issues.apache.org/jira/browse/KUDU-2049>
.
-Will
On Thu, Jun 22, 2017 at 10:23 AM, Todd Lipcon <to...@cloudera.com> wrote:
> Mind filling a Jira for this since it seems like the kind of bug we will
> want to backport?
>
> On Jun 22, 2017 9:45 AM, "Will Berkeley (Code Review)" <
> gerrit@cloudera.org> wrote:
>
>> Will Berkeley has uploaded a new change for review.
>>
>> http://gerrit.cloudera.org:8080/7262
>>
>> Change subject: Fix too-strict CHECK in RleIntBlockDecoder::SeekToPosi
>> tionInBlock
>> ......................................................................
>>
>> Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
>>
>> In a CFile block with n non-null values, it's permissible to
>> seek to the spot after all rows. However, a CHECK_LT in
>> RleIntBlockDecoder::SeekToPositionInBlock was enforcing seeks to
>> be < n. This patches switches the CHECK_LT to a DCHECK_LE,
>> making it consistent with the checks in other decoders. The
>> problem was not caught in testing due to an omission of RLE
>> encoding from cfile-test.cc's
>> TestCFileBothCacheTypes.TestNullInts. The test now has coverage
>> of RLE encoding and, with slow tests on, fails every time
>> without the change to the CHECK.
>>
>> Additionally, TestCFileBothCacheTypes.TestNullFloats was missing
>> coverage for bit shuffle encoding. This was also fixed.
>>
>> Finally, RleBitMapBlockDecoder::SeekToPositionInBlock was
>> missing any check on its 'pos' argument, so the checks from
>> RleIntBlockDecoder::SeekToPositionInBlock were added to that
>> method as well.
>>
>> Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
>> ---
>> M src/kudu/cfile/cfile-test.cc
>> M src/kudu/cfile/rle_block.h
>> 2 files changed, 13 insertions(+), 3 deletions(-)
>>
>>
>> git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7262/1
>> --
>> To view, visit http://gerrit.cloudera.org:8080/7262
>> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>>
>> Gerrit-MessageType: newchange
>> Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
>> Gerrit-PatchSet: 1
>> Gerrit-Project: kudu
>> Gerrit-Branch: master
>> Gerrit-Owner: Will Berkeley <wd...@gmail.com>
>>
>
Re: [kudu-CR] Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by Todd Lipcon <to...@cloudera.com>.
Mind filling a Jira for this since it seems like the kind of bug we will
want to backport?
On Jun 22, 2017 9:45 AM, "Will Berkeley (Code Review)" <ge...@cloudera.org>
wrote:
> Will Berkeley has uploaded a new change for review.
>
> http://gerrit.cloudera.org:8080/7262
>
> Change subject: Fix too-strict CHECK in RleIntBlockDecoder::
> SeekToPositionInBlock
> ......................................................................
>
> Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
>
> In a CFile block with n non-null values, it's permissible to
> seek to the spot after all rows. However, a CHECK_LT in
> RleIntBlockDecoder::SeekToPositionInBlock was enforcing seeks to
> be < n. This patches switches the CHECK_LT to a DCHECK_LE,
> making it consistent with the checks in other decoders. The
> problem was not caught in testing due to an omission of RLE
> encoding from cfile-test.cc's
> TestCFileBothCacheTypes.TestNullInts. The test now has coverage
> of RLE encoding and, with slow tests on, fails every time
> without the change to the CHECK.
>
> Additionally, TestCFileBothCacheTypes.TestNullFloats was missing
> coverage for bit shuffle encoding. This was also fixed.
>
> Finally, RleBitMapBlockDecoder::SeekToPositionInBlock was
> missing any check on its 'pos' argument, so the checks from
> RleIntBlockDecoder::SeekToPositionInBlock were added to that
> method as well.
>
> Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
> ---
> M src/kudu/cfile/cfile-test.cc
> M src/kudu/cfile/rle_block.h
> 2 files changed, 13 insertions(+), 3 deletions(-)
>
>
> git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7262/1
> --
> To view, visit http://gerrit.cloudera.org:8080/7262
> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>
> Gerrit-MessageType: newchange
> Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
> Gerrit-PatchSet: 1
> Gerrit-Project: kudu
> Gerrit-Branch: master
> Gerrit-Owner: Will Berkeley <wd...@gmail.com>
>
[kudu-CR] KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7262/2/src/kudu/cfile/rle_block.h
File src/kudu/cfile/rle_block.h:
PS2, Line 145: .
nit: drop the dot
PS2, Line 342: .
nit: drop the dot :)
PS2, Line 340: DCHECK_LE(pos, num_elems_)
: << "Tried to seek to " << pos << " which is > number of elements ("
: << num_elems_ << ") in the block!.";
What would happen in release build if, say, pos == num_elems_ + 1 ? Is it going to be an undefined behavior which would lead to silent data corruption or it's caught by some other sanity check?
--
To view, visit http://gerrit.cloudera.org:8080/7262
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae72ca1d2af06de1e77cde233e98aa6af97d07e8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes