Skip to content

Commit

Permalink
core: fix corner cases with less than comparison (#1748)
Browse files Browse the repository at this point in the history 
For the roaring tag index there were some bugs resulting
in incorrect matching if the value being compared was larger
than the last element for a given tag. The offset position
was for the last tag and so it would return an empty match
set since the tag key didn't match.
  • Loading branch information
@brharrington
brharrington authored Feb 19, 2025
1 parent 155e802 commit 5bd1b9d
Show file tree
Hide file tree
Showing 2 changed files with 193 additions and 9 deletions.
36 changes: 27 additions & 9 deletions atlas-core/src/main/scala/com/netflix/atlas/core/index/RoaringTagIndex.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,14 +276,16 @@ class RoaringTagIndex[T <: TaggedItem](items: Array[T], stats: IndexStats) exten
if (vidx == null) new RoaringBitmap()
else {
val set = new RoaringBitmap()
val vp = findOffset(values, v, if (orEqual) 0 else -1)
val t = tag(kp, vp)
var i = tagOffset(t)
val vp = findOffsetLessThan(values, v, if (orEqual) 0 else -1)
if (vp >= 0) {
val t = tag(kp, vp)
var i = tagOffset(t)

// Data is sorted, no need to perform a check for each entry if key matches
while (i >= 0 && tagKey(tagIndex(i)) == kp) {
set.or(vidx.get(tagValue(tagIndex(i))))
i -= 1
// Data is sorted, no need to perform a check for each entry if key matches
while (i >= 0 && tagKey(tagIndex(i)) == kp) {
set.or(vidx.get(tagValue(tagIndex(i))))
i -= 1
}
}
set
}
Expand Down Expand Up @@ -347,8 +349,8 @@ class RoaringTagIndex[T <: TaggedItem](items: Array[T], stats: IndexStats) exten
/**
* Find the offset for `v` in the array `vs`. If an exact match is found, then
* the value `n` will be added to the position. This is mostly used for skipping
* equal values in the case of strictly less than or greater than comparisons. By
* default a greater than, `n = 1`, comparison will be done.
* equal values in the case of strictly greater than comparisons. By default a
* greater than, `n = 1`, comparison will be done.
*/
private def findOffset(vs: Array[String], v: String, n: Int = 1): Int = {
if (v == null || v == "") 0
Expand All @@ -358,6 +360,22 @@ class RoaringTagIndex[T <: TaggedItem](items: Array[T], stats: IndexStats) exten
}
}

/**
* Find the offset for `v` in the array `vs`. If an exact match is found, then
* the value `n` will be added to the position. This is mostly used for skipping
* equal values in the case of strictly less than. If no match is found, then it
* will be the position where the next item less than the value should be.
*/
private def findOffsetLessThan(vs: Array[String], v: String, n: Int): Int = {
if (v == null || v == "") 0
else {
// Binary search gives position of item if not found, need to skip one position
// for the position of the item less than.
val pos = util.Arrays.binarySearch(vs.asInstanceOf[Array[AnyRef]], v)
if (pos >= 0) pos + n else -pos - 2
}
}

def findTags(query: TagQuery): List[Tag] = {
val k = query.key
if (k.isDefined) {
Expand Down
166 changes: 166 additions & 0 deletions atlas-core/src/test/scala/com/netflix/atlas/core/index/RoaringTagIndexSuite.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
*/
package com.netflix.atlas.core.index

import com.netflix.atlas.core.model.BasicTaggedItem
import com.netflix.atlas.core.model.Datapoint
import com.netflix.atlas.core.model.Query
import com.netflix.atlas.core.model.TimeSeries
import org.roaringbitmap.RoaringBitmap

Expand Down Expand Up @@ -60,4 +62,168 @@ class RoaringTagIndexSuite extends TagIndexSuite {
b2.add(20)
assert(RoaringTagIndex.hasNonEmptyIntersection(b1, b2))
}

private def createLessThanIndex: TagIndex[BasicTaggedItem] = {
val items = (0 until 10).map { i =>
val tags = Map(
"name" -> "test",
"a" -> i.toString,
"b" -> "foo"
)
BasicTaggedItem(tags)
}
RoaringTagIndex(items.toArray, new IndexStats())
}

test("lt, first element for tag") {
val idx = createLessThanIndex
val q = Query.LessThan("a", "0")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("a")).toSet, Set.empty[String])
}

test("lt, second element for tag") {
val idx = createLessThanIndex
val q = Query.LessThan("a", "1")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("a")).toSet, Set("0"))
}

test("lt, missing element after first for tag") {
val idx = createLessThanIndex
val q = Query.LessThan("a", "00")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("a")).toSet, Set("0"))
}

test("lt, last element for tag") {
val idx = createLessThanIndex
val q = Query.LessThan("a", "9")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("a")).toSet, (0 until 9).map(_.toString).toSet)
}

test("lt, after last element for tag") {
val idx = createLessThanIndex
val q = Query.LessThan("a", "a")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("a")).toSet, (0 until 10).map(_.toString).toSet)
}

test("le, first element for tag") {
val idx = createLessThanIndex
val q = Query.LessThanEqual("a", "0")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("a")).toSet, Set("0"))
}

test("le, second element for tag") {
val idx = createLessThanIndex
val q = Query.LessThanEqual("a", "1")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("a")).toSet, Set("0", "1"))
}

test("le, missing element after first for tag") {
val idx = createLessThanIndex
val q = Query.LessThanEqual("a", "00")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("a")).toSet, Set("0"))
}

test("le, last element for tag") {
val idx = createLessThanIndex
val q = Query.LessThanEqual("a", "9")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("a")).toSet, (0 until 10).map(_.toString).toSet)
}

test("le, after last element for tag") {
val idx = createLessThanIndex
val q = Query.LessThanEqual("a", "a")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("a")).toSet, (0 until 10).map(_.toString).toSet)
}

private def createGreaterThanIndex: TagIndex[BasicTaggedItem] = {
val items = (0 until 10).map { i =>
val tags = Map(
"name" -> "test",
"z" -> i.toString,
"b" -> "foo"
)
BasicTaggedItem(tags)
}
RoaringTagIndex(items.toArray, new IndexStats())
}

test("gt, first element for tag") {
val idx = createGreaterThanIndex
val q = Query.GreaterThan("z", "0")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("z")).toSet, (1 until 10).map(_.toString).toSet)
}

test("gt, second element for tag") {
val idx = createGreaterThanIndex
val q = Query.GreaterThan("z", "1")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("z")).toSet, (2 until 10).map(_.toString).toSet)
}

test("gt, missing element after first for tag") {
val idx = createGreaterThanIndex
val q = Query.GreaterThan("z", "00")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("z")).toSet, (1 until 10).map(_.toString).toSet)
}

test("gt, last element for tag") {
val idx = createGreaterThanIndex
val q = Query.GreaterThan("z", "9")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("z")).toSet, Set.empty[String])
}

test("gt, after last element for tag") {
val idx = createGreaterThanIndex
val q = Query.GreaterThan("z", "a")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("z")).toSet, Set.empty[String])
}

test("ge, first element for tag") {
val idx = createGreaterThanIndex
val q = Query.GreaterThanEqual("z", "0")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("z")).toSet, (0 until 10).map(_.toString).toSet)
}

test("ge, second element for tag") {
val idx = createGreaterThanIndex
val q = Query.GreaterThanEqual("z", "1")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("z")).toSet, (1 until 10).map(_.toString).toSet)
}

test("ge, missing element after first for tag") {
val idx = createGreaterThanIndex
val q = Query.GreaterThanEqual("z", "00")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("z")).toSet, (1 until 10).map(_.toString).toSet)
}

test("ge, last element for tag") {
val idx = createGreaterThanIndex
val q = Query.GreaterThanEqual("z", "9")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("z")).toSet, Set("9"))
}

test("ge, after last element for tag") {
val idx = createGreaterThanIndex
val q = Query.GreaterThanEqual("z", "a")
val rs = idx.findItems(TagQuery(Some(q)))
assertEquals(rs.map(_.tags("z")).toSet, Set.empty[String])
}
}

0 comments on commit 5bd1b9d

Please sign in to comment.