From 5bd1b9d561d334cc4f436e0a67f00eed94a7adb6 Mon Sep 17 00:00:00 2001 From: brharrington Date: Wed, 19 Feb 2025 10:45:46 -0600 Subject: [PATCH] core: fix corner cases with less than comparison (#1748) 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. --- .../atlas/core/index/RoaringTagIndex.scala | 36 +++- .../core/index/RoaringTagIndexSuite.scala | 166 ++++++++++++++++++ 2 files changed, 193 insertions(+), 9 deletions(-) diff --git a/atlas-core/src/main/scala/com/netflix/atlas/core/index/RoaringTagIndex.scala b/atlas-core/src/main/scala/com/netflix/atlas/core/index/RoaringTagIndex.scala index 509762235..7fa8c76cb 100644 --- a/atlas-core/src/main/scala/com/netflix/atlas/core/index/RoaringTagIndex.scala +++ b/atlas-core/src/main/scala/com/netflix/atlas/core/index/RoaringTagIndex.scala @@ -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 } @@ -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 @@ -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) { diff --git a/atlas-core/src/test/scala/com/netflix/atlas/core/index/RoaringTagIndexSuite.scala b/atlas-core/src/test/scala/com/netflix/atlas/core/index/RoaringTagIndexSuite.scala index bfd24feef..d07751aff 100644 --- a/atlas-core/src/test/scala/com/netflix/atlas/core/index/RoaringTagIndexSuite.scala +++ b/atlas-core/src/test/scala/com/netflix/atlas/core/index/RoaringTagIndexSuite.scala @@ -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 @@ -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]) + } }