kudu git commit: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock

[wd...@apache.org]

Repository: kudu
Updated Branches:
  refs/heads/master c25d127ad -> f9e51ca63


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


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/f9e51ca6
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/f9e51ca6
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/f9e51ca6

Branch: refs/heads/master
Commit: f9e51ca636fdcc29071be7f76868fa7b4509803d
Parents: c25d127
Author: Will Berkeley <wd...@apache.org>
Authored: Thu Jun 22 08:42:23 2017 -0700
Committer: Will Berkeley <wd...@gmail.com>
Committed: Fri Jun 23 19:37:19 2017 +0000

----------------------------------------------------------------------
 src/kudu/cfile/cfile-test.cc |  3 +++
 src/kudu/cfile/rle_block.h   | 13 ++++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/f9e51ca6/src/kudu/cfile/cfile-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/cfile-test.cc b/src/kudu/cfile/cfile-test.cc
index e21d96e..dc29a62 100644
--- a/src/kudu/cfile/cfile-test.cc
+++ b/src/kudu/cfile/cfile-test.cc
@@ -859,11 +859,14 @@ TEST_P(TestCFileBothCacheTypes, TestNullInts) {
   TestNullTypes(&generator, PLAIN_ENCODING, LZ4);
   TestNullTypes(&generator, BIT_SHUFFLE, NO_COMPRESSION);
   TestNullTypes(&generator, BIT_SHUFFLE, LZ4);
+  TestNullTypes(&generator, RLE, NO_COMPRESSION);
+  TestNullTypes(&generator, RLE, LZ4);
 }
 
 TEST_P(TestCFileBothCacheTypes, TestNullFloats) {
   FPDataGenerator<FLOAT, true> generator;
   TestNullTypes(&generator, PLAIN_ENCODING, NO_COMPRESSION);
+  TestNullTypes(&generator, BIT_SHUFFLE, NO_COMPRESSION);
 }
 
 TEST_P(TestCFileBothCacheTypes, TestNullPrefixStrings) {

http://git-wip-us.apache.org/repos/asf/kudu/blob/f9e51ca6/src/kudu/cfile/rle_block.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/rle_block.h b/src/kudu/cfile/rle_block.h
index 6918a2c..4cc6e02 100644
--- a/src/kudu/cfile/rle_block.h
+++ b/src/kudu/cfile/rle_block.h
@@ -140,6 +140,13 @@ class RleBitMapBlockDecoder final : public BlockDecoder {
 
   virtual void SeekToPositionInBlock(uint pos) OVERRIDE {
     CHECK(parsed_) << "Must call ParseHeader()";
+    DCHECK_LE(pos, num_elems_)
+      << "Tried to seek to " << pos << " which is > number of elements ("
+      << num_elems_ << ") in the block!";
+    // If the block is empty (e.g. the column is filled with nulls), there is no data to seek.
+    if (PREDICT_FALSE(num_elems_ == 0)) {
+      return;
+    }
 
     if (cur_idx_ == pos) {
       // No need to seek.
@@ -330,13 +337,13 @@ class RleIntBlockDecoder final : public BlockDecoder {
 
   virtual void SeekToPositionInBlock(uint pos) OVERRIDE {
     CHECK(parsed_) << "Must call ParseHeader()";
+    DCHECK_LE(pos, num_elems_)
+        << "Tried to seek to " << pos << " which is > number of elements ("
+        << num_elems_ << ") in the block!";
     // If the block is empty (e.g. the column is filled with nulls), there is no data to seek.
     if (PREDICT_FALSE(num_elems_ == 0)) {
       return;
     }
-    CHECK_LT(pos, num_elems_)
-        << "Tried to seek to " << pos << " which is >= number of elements ("
-        << num_elems_ << ") in the block!.";
 
     if (cur_idx_ == pos) {
       // No need to seek.