Skip to content

Commit 3f77407

Browse files
committed
fix: Bug GEQ range query bug (#4557)
The bug was caused by incorrect handling of corner cases, when a path that should lead to an item was wrongfully cleared, which lead to empty results for SortedMap::GetRange query. This PR: 1. fixes the wrong code in bptree_set.h. 2. Adds unit tests for both bptree_set_test and sorted_map_test. Related to mastodon/mastodon#33805 Signed-off-by: Roman Gershman <roman@dragonflydb.io>
1 parent 980af20 commit 3f77407

File tree

3 files changed

+53
-4
lines changed

3 files changed

+53
-4
lines changed

src/core/bptree_set.h

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -489,8 +489,14 @@ template <typename T, typename Policy>
489489
detail::BPTreePath<T> BPTree<T, Policy>::GEQ(KeyT item) const {
490490
BPTreePath path;
491491

492-
if (!Locate(item, &path) && path.Last().second >= path.Last().first->NumItems())
493-
path.Clear();
492+
bool res = Locate(item, &path);
493+
494+
// if we did not find the item and the path does not lead to any key in the node,
495+
// adjust the path to point to the next key in the tree.
496+
// In case we are past all items in the tree, Next() will collapse to the empty path.
497+
if (!res && path.Last().second >= path.Last().first->NumItems()) {
498+
path.Next();
499+
}
494500

495501
return path;
496502
}

src/core/bptree_set_test.cc

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,12 +88,16 @@ class BPTreeSetTest : public ::testing::Test {
8888
static void SetUpTestSuite() {
8989
}
9090

91-
void FillTree(unsigned factor = 1) {
92-
for (unsigned i = 0; i < kNumElems; ++i) {
91+
void FillTree(unsigned start, unsigned factor) {
92+
for (unsigned i = start; i < kNumElems; ++i) {
9393
bptree_.Insert(i * factor);
9494
}
9595
}
9696

97+
void FillTree(unsigned factor = 1) {
98+
FillTree(0, factor);
99+
}
100+
97101
bool Validate();
98102

99103
MiMemoryResource mi_alloc_;
@@ -277,6 +281,25 @@ TEST_F(BPTreeSetTest, Ranges) {
277281
EXPECT_TRUE(path.Empty());
278282
}
279283

284+
TEST_F(BPTreeSetTest, HalfRanges) {
285+
FillTree(1, 3); // 3, 6, 9 ...
286+
auto path = bptree_.FromRank(bptree_.Size() - 1);
287+
uint64_t val = path.Terminal();
288+
for (unsigned i = 0; i <= val; ++i) {
289+
path = bptree_.GEQ(i);
290+
ASSERT_FALSE(path.Empty()) << i;
291+
}
292+
path = bptree_.GEQ(val + 1);
293+
ASSERT_TRUE(path.Empty());
294+
295+
for (unsigned i = 3; i <= val + 10; ++i) {
296+
path = bptree_.LEQ(i);
297+
ASSERT_FALSE(path.Empty()) << i;
298+
}
299+
path = bptree_.LEQ(2);
300+
ASSERT_TRUE(path.Empty());
301+
}
302+
280303
TEST_F(BPTreeSetTest, MemoryUsage) {
281304
zskiplist* zsl = zslCreate();
282305
std::vector<sds> sds_vec;

src/core/sorted_map_test.cc

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,26 @@ TEST_F(SortedMapTest, DeleteRange) {
246246
EXPECT_EQ(96, sm_.DeleteRangeByLex(lex_range));
247247
}
248248

249+
TEST_F(SortedMapTest, RangeBug) {
250+
constexpr size_t kArrLen = 80;
251+
for (unsigned i = 0; i < kArrLen; i++) {
252+
sds s = sdsempty();
253+
254+
s = sdscatfmt(s, "score%u", i);
255+
sm_.Insert(i, s);
256+
}
257+
258+
for (unsigned i = 0; i < kArrLen; i++) {
259+
zrangespec range;
260+
range.max = HUGE_VAL;
261+
range.min = i;
262+
range.minex = 0;
263+
range.maxex = 0;
264+
auto arr = sm_.GetRange(range, 0, 5, false);
265+
ASSERT_GT(arr.size(), 0) << i;
266+
}
267+
}
268+
249269
// not a real test, just to see how much memory is used by zskiplist.
250270
TEST_F(SortedMapTest, MemoryUsage) {
251271
zskiplist* zsl = zslCreate();

0 commit comments

Comments
 (0)