kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From mpe...@apache.org
Subject kudu git commit: KUDU-2049 Fix too-strict CHECK in RleIntBlockDecoder::SeekToPositionInBlock
Date Wed, 28 Jun 2017 18:43:32 GMT
Repository: kudu
Updated Branches:
  refs/heads/branch-1.2.x b22bd3358 -> 3955445ad


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 <aserbin@cloudera.com>
Reviewed-by: Andrew Wong <awong@cloudera.com>
Tested-by: Kudu Jenkins
(cherry picked from commit f9e51ca636fdcc29071be7f76868fa7b4509803d)
Reviewed-on: http://gerrit.cloudera.org:8080/7317
Reviewed-by: Todd Lipcon <todd@apache.org>
Tested-by: Will Berkeley <wdberkeley@gmail.com>


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

Branch: refs/heads/branch-1.2.x
Commit: 3955445ad1ecf9535f09e9640db4da0f49be521e
Parents: b22bd33
Author: Will Berkeley <wdberkeley@apache.org>
Authored: Thu Jun 22 08:42:23 2017 -0700
Committer: Will Berkeley <wdberkeley@gmail.com>
Committed: Wed Jun 28 17:51:17 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/3955445a/src/kudu/cfile/cfile-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/cfile-test.cc b/src/kudu/cfile/cfile-test.cc
index 1db6b76..9040bc3 100644
--- a/src/kudu/cfile/cfile-test.cc
+++ b/src/kudu/cfile/cfile-test.cc
@@ -744,11 +744,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/3955445a/src/kudu/cfile/rle_block.h
----------------------------------------------------------------------
diff --git a/src/kudu/cfile/rle_block.h b/src/kudu/cfile/rle_block.h
index c8ed76b..c6cdbaa 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.


Mime
View raw message