Merge "Fix sstable reader not working for empty set of clustering ranges" from Tomasz

"Fixes #2734."

* 'tgrabiec/make-sstable-reader-work-with-empty-range-set' of github.com:scylladb/seastar-dev:
  tests: Introduce clustering_ranges_walker_test
  tests: simple_schema: Add missing include
  sstables: reader: Make clustering_ranges_walker work with empty range set
  clustering_ranges_walker: Make adjacency more accurate

clustering_ranges_walker.hh

@@ -43,10 +43,14 @@ private:
     bool advance_to_next_range() {
         _in_current = false;
         if (!_current_start.is_static_row()) {
+            if (_current == _end) {
+                return false;
+            }
             ++_current;
         }
         ++_change_counter;
         if (_current == _end) {
+            _current_end = _current_start = position_in_partition_view::after_all_clustered_rows();
             return false;
         }
         _current_start = position_in_partition_view::for_range_start(*_current);
@@ -61,11 +65,18 @@ public:
         , _end(ranges.end())
         , _in_current(with_static_row)
         , _with_static_row(with_static_row)
-        , _current_start(with_static_row ? position_in_partition_view::for_static_row()
-                                         : position_in_partition_view::for_range_start(*_current))
-        , _current_end(with_static_row ? position_in_partition_view::before_all_clustered_rows()
-                                       : position_in_partition_view::for_range_end(*_current))
-    { }
+        , _current_start(position_in_partition_view::for_static_row())
+        , _current_end(position_in_partition_view::before_all_clustered_rows())
+    {
+        if (!with_static_row) {
+            if (_current == _end) {
+                _current_start = _current_end = position_in_partition_view::after_all_clustered_rows();
+            } else {
+                _current_start = position_in_partition_view::for_range_start(*_current);
+                _current_end = position_in_partition_view::for_range_end(*_current);
+            }
+        }
+    }
     clustering_ranges_walker(clustering_ranges_walker&& o) noexcept
         : _schema(o._schema)
         , _ranges(o._ranges)
@@ -94,10 +105,6 @@ public:
     void trim_front(position_in_partition pos) {
         position_in_partition::less_compare less(_schema);
 
-        if (_current == _end) {
-            return;
-        }
-
         do {
             if (!less(_current_start, pos)) {
                 break;
@@ -118,10 +125,6 @@ public:
     bool advance_to(position_in_partition_view pos) {
         position_in_partition::less_compare less(_schema);
 
-        if (_current == _end) {
-            return false;
-        }
-
         do {
             if (!_in_current && less(pos, _current_start)) {
                 break;
@@ -146,12 +149,8 @@ public:
     bool advance_to(position_in_partition_view start, position_in_partition_view end) {
         position_in_partition::less_compare less(_schema);
 
-        if (_current == _end) {
-            return false;
-        }
-
         do {
-            if (less(end, _current_start)) {
+            if (!less(_current_start, end)) {
                 break;
             }
             if (less(start, _current_end)) {
@@ -192,7 +191,7 @@ public:
 
     // Returns true if advanced past all contained positions. Any later advance_to() until reset() will return false.
     bool out_of_range() const {
-        return _current == _end;
+        return !_in_current && _current == _end;
     }
 
     // Resets the state of the walker so that advance_to() can be now called for new sequence of positions.

configure.py

@@ -176,6 +176,7 @@ scylla_tests = [
     'tests/partitioner_test',
     'tests/frozen_mutation_test',
     'tests/serialized_action_test',
+    'tests/clustering_ranges_walker_test',
     'tests/perf/perf_mutation',
     'tests/lsa_async_eviction_test',
     'tests/lsa_sync_eviction_test',

test.py

@@ -83,6 +83,7 @@ boost_tests = [
     'cell_locker_test',
     'view_schema_test',
     'combined_mutation_reader_test',
+    'clustering_ranges_walker_test',
 ]
 
 other_tests = [

tests/clustering_ranges_walker_test.cc

@@ -0,0 +1,296 @@
+/*
+ * Copyright (C) 2017 ScyllaDB
+ */
+
+/*
+ * This file is part of Scylla.
+ *
+ * Scylla is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * Scylla is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with Scylla.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+
+#include <boost/test/unit_test.hpp>
+#include "tests/test-utils.hh"
+
+#include "simple_schema.hh"
+#include "clustering_ranges_walker.hh"
+
+#include "disk-error-handler.hh"
+
+thread_local disk_error_signal_type commit_error;
+thread_local disk_error_signal_type general_disk_error;
+
+using namespace std::chrono_literals;
+
+struct step {
+    position_in_partition_view pos;
+    bool contained;
+};
+
+struct data_set {
+    schema_ptr schema;
+    query::clustering_row_ranges ranges;
+    position_in_partition end_pos; // all >= than this is outside the range. TODO: derive from ranges
+    std::vector<step> steps; // ordered by step::pos
+    bool include_static_row = true;
+};
+
+static void run_tests(data_set ds) {
+    position_in_partition::tri_compare cmp(*ds.schema);
+
+    {
+        clustering_ranges_walker walker(*ds.schema, ds.ranges, ds.include_static_row);
+
+        auto check_all = [&] {
+            for (auto&& e : ds.steps) {
+                auto&& key = e.pos;
+                BOOST_REQUIRE(walker.advance_to(key) == e.contained);
+                BOOST_REQUIRE(walker.advance_to(key) == e.contained); // should be idempotent
+                BOOST_REQUIRE(walker.out_of_range() == (cmp(key, ds.end_pos) >= 0));
+            }
+            BOOST_REQUIRE(walker.out_of_range());
+        };
+
+        check_all();
+        walker.reset();
+        check_all();
+    }
+
+    // check advance_to() on all pairs of steps
+    {
+        for (unsigned i = 0; i < ds.steps.size(); ++i) {
+            for (unsigned j = i; j < ds.steps.size(); ++j) {
+                clustering_ranges_walker walker(*ds.schema, ds.ranges, ds.include_static_row);
+                BOOST_REQUIRE(walker.advance_to(ds.steps[i].pos) == ds.steps[i].contained);
+                BOOST_REQUIRE(walker.out_of_range() == (cmp(ds.steps[i].pos, ds.end_pos) >= 0));
+                BOOST_REQUIRE(walker.advance_to(ds.steps[j].pos) == ds.steps[j].contained);
+                BOOST_REQUIRE(walker.out_of_range() == (cmp(ds.steps[j].pos, ds.end_pos) >= 0));
+                BOOST_REQUIRE(!walker.advance_to(position_in_partition_view::after_all_clustered_rows()));
+                BOOST_REQUIRE(walker.out_of_range());
+            }
+        }
+    }
+
+    // check advance_to() in case of trim_front()
+    {
+        for (unsigned i = 0; i < ds.steps.size(); ++i) {
+            for (unsigned j = i; j < ds.steps.size(); ++j) {
+                clustering_ranges_walker walker(*ds.schema, ds.ranges, ds.include_static_row);
+                if (i) {
+                    walker.advance_to(ds.steps[i - 1].pos);
+                }
+
+                walker.trim_front(position_in_partition(ds.steps[j].pos));
+
+                // check that all before j are excluded
+                for (unsigned k = i; k < j; ++k) {
+                    BOOST_REQUIRE(!walker.advance_to(ds.steps[k].pos));
+                }
+
+                // check that all after j are as expected
+                for (unsigned k = j; k < ds.steps.size(); ++k) {
+                    BOOST_REQUIRE(walker.advance_to(ds.steps[k].pos) == ds.steps[k].contained);
+                    BOOST_REQUIRE(walker.out_of_range() == (cmp(ds.steps[k].pos, ds.end_pos) >= 0));
+                }
+            }
+        }
+    }
+}
+
+SEASTAR_TEST_CASE(test_basic_operation_on_various_data_sets) {
+    simple_schema s;
+    auto keys = s.make_ckeys(10);
+
+    {
+        auto ranges = query::clustering_row_ranges({
+            query::clustering_range::make({keys[1], true}, {keys[1], true}),
+            query::clustering_range::make({keys[2], false}, {keys[3], false}),
+            query::clustering_range::make({keys[4], true}, {keys[6], false}),
+            query::clustering_range::make({keys[6], true}, {keys[6], true}),
+        });
+
+        auto end_pos = position_in_partition::after_key(keys[6]);
+
+        auto steps = std::vector<step>({
+            {position_in_partition_view::for_static_row(),            true},
+            {position_in_partition_view::before_all_clustered_rows(), false},
+            {position_in_partition_view::for_key(keys[1]),            true},
+            {position_in_partition_view::for_key(keys[2]),            false},
+            {position_in_partition_view::for_key(keys[3]),            false},
+            {position_in_partition_view::for_key(keys[4]),            true},
+            {position_in_partition_view::for_key(keys[5]),            true},
+            {position_in_partition_view::for_key(keys[6]),            true},
+            {position_in_partition_view::for_key(keys[7]),            false},
+            {position_in_partition_view::after_all_clustered_rows(),  false},
+        });
+
+        run_tests(data_set{s.schema(), ranges, end_pos, steps});
+    }
+
+    {
+        auto ranges = query::clustering_row_ranges({
+            query::clustering_range::make({keys[1], false}, {keys[2], true}),
+        });
+
+        auto end_pos = position_in_partition::after_key(keys[2]);
+
+        auto steps = std::vector<step>({
+            {position_in_partition_view::for_static_row(),            true},
+            {position_in_partition_view::before_all_clustered_rows(), false},
+            {position_in_partition_view::for_key(keys[1]),            false},
+            {position_in_partition_view::for_key(keys[2]),            true},
+            {position_in_partition_view::after_all_clustered_rows(),  false},
+        });
+
+        run_tests(data_set{s.schema(), ranges, end_pos, steps});
+    }
+
+    {
+        auto ranges = query::clustering_row_ranges({
+            query::clustering_range::make({keys[1], false}, {keys[2], true}),
+        });
+
+        auto end_pos = position_in_partition::after_key(keys[2]);
+
+        auto steps = std::vector<step>({
+            {position_in_partition_view::for_static_row(),            false},
+            {position_in_partition_view::before_all_clustered_rows(), false},
+            {position_in_partition_view::for_key(keys[1]),            false},
+            {position_in_partition_view::for_key(keys[2]),            true},
+            {position_in_partition_view::after_all_clustered_rows(),  false},
+        });
+
+        bool include_static_row = false;
+
+        run_tests(data_set{s.schema(), ranges, end_pos, steps, include_static_row});
+    }
+
+    {
+        auto ranges = query::clustering_row_ranges({});
+
+        auto end_pos = position_in_partition(position_in_partition_view::before_all_clustered_rows());
+
+        auto steps = std::vector<step>({
+            {position_in_partition_view::for_static_row(),            true},
+            {position_in_partition_view::before_all_clustered_rows(), false},
+            {position_in_partition_view::for_key(keys[1]),            false},
+            {position_in_partition_view::after_all_clustered_rows(),  false},
+        });
+
+        run_tests(data_set{s.schema(), ranges, end_pos, steps});
+    }
+
+    {
+        auto ranges = query::clustering_row_ranges({});
+
+        auto end_pos = position_in_partition(position_in_partition_view::for_static_row());
+
+        auto steps = std::vector<step>({
+            {position_in_partition_view::for_static_row(),            false},
+            {position_in_partition_view::before_all_clustered_rows(), false},
+            {position_in_partition_view::for_key(keys[1]),            false},
+            {position_in_partition_view::after_all_clustered_rows(),  false},
+        });
+
+        bool include_static_row = false;
+
+        run_tests(data_set{s.schema(), ranges, end_pos, steps, include_static_row});
+    }
+
+    return make_ready_future<>();
+}
+
+SEASTAR_TEST_CASE(test_range_overlap) {
+    simple_schema s;
+    auto keys = s.make_ckeys(10);
+
+    auto range1 = query::clustering_range::make({keys[1], true}, {keys[2], false});
+    auto range2 = query::clustering_range::make({keys[4], true}, {keys[6], false});
+    auto ranges = query::clustering_row_ranges({range1, range2});
+
+    clustering_ranges_walker walker(*s.schema(), ranges);
+    position_in_partition::equal_compare eq(*s.schema());
+
+    auto lbcc = walker.lower_bound_change_counter();
+
+    BOOST_REQUIRE(eq(walker.lower_bound(), position_in_partition(position_in_partition_view::for_static_row())));
+
+    BOOST_REQUIRE(walker.advance_to(
+        position_in_partition(position_in_partition_view::for_static_row()),
+        position_in_partition::for_key(keys[1])));
+
+    BOOST_REQUIRE(!walker.advance_to(
+        position_in_partition::for_key(keys[0]),
+        position_in_partition::after_key(keys[0])));
+
+    ++lbcc;
+    BOOST_REQUIRE(walker.lower_bound_change_counter() == lbcc);
+    BOOST_REQUIRE(eq(walker.lower_bound(), position_in_partition::for_range_start(range1)));
+
+    BOOST_REQUIRE(walker.advance_to(
+        position_in_partition::after_key(keys[0]),
+        position_in_partition::for_key(keys[1])));
+
+    BOOST_REQUIRE(walker.lower_bound_change_counter() == lbcc);
+    BOOST_REQUIRE(eq(walker.lower_bound(), position_in_partition::for_range_start(range1)));
+
+    BOOST_REQUIRE(walker.advance_to(
+        position_in_partition::for_key(keys[1]),
+        position_in_partition::for_key(keys[7])));
+
+    BOOST_REQUIRE(!walker.advance_to(
+        position_in_partition::for_key(keys[2]),
+        position_in_partition::for_key(keys[3])));
+
+    ++lbcc;
+    BOOST_REQUIRE(walker.lower_bound_change_counter() == lbcc);
+    BOOST_REQUIRE(eq(walker.lower_bound(), position_in_partition::for_range_start(range2)));
+
+    BOOST_REQUIRE(walker.advance_to(
+        position_in_partition::after_key(keys[2]),
+        position_in_partition::for_key(keys[4])));
+
+    BOOST_REQUIRE(walker.advance_to(
+        position_in_partition::for_key(keys[3]),
+        position_in_partition::for_key(keys[7])));
+
+    BOOST_REQUIRE(walker.advance_to(
+        position_in_partition::for_key(keys[4]),
+        position_in_partition::for_key(keys[4])));
+
+    BOOST_REQUIRE(!walker.advance_to(
+        position_in_partition::for_key(keys[6]),
+        position_in_partition::for_key(keys[7])));
+
+    ++lbcc;
+    BOOST_REQUIRE(walker.lower_bound_change_counter() == lbcc);
+    BOOST_REQUIRE(walker.out_of_range());
+
+    BOOST_REQUIRE(!walker.advance_to(
+        position_in_partition::for_key(keys[7]),
+        position_in_partition::for_key(keys[7])));
+
+    BOOST_REQUIRE(!walker.advance_to(
+        position_in_partition::for_key(keys[8]),
+        position_in_partition::after_all_clustered_rows()));
+
+    BOOST_REQUIRE(!walker.advance_to(
+        position_in_partition::after_all_clustered_rows(),
+        position_in_partition::after_all_clustered_rows()));
+
+    BOOST_REQUIRE(walker.lower_bound_change_counter() == lbcc);
+    BOOST_REQUIRE(walker.out_of_range());
+
+    return make_ready_future<>();
+}

tests/make_random_string.hh

@@ -24,6 +24,7 @@
 #pragma once
 
 #include <seastar/core/sstring.hh>
+#include "seastarx.hh"
 #include <random>
 
 inline