CNDB-13830: Fix KD-tree metrics test (#1778)

The test for KD-tree posting list was meant to run a query that
performs skips and then verifies if the skip metric has been
bumped up.

Unfortunately there were several problems with it:
* the query didn't do skips because the clause intersection got
  optimized out by the SAI optimizer
* the test for the metric didn't inspect the metric value, but only
  checked if the metric was updated; so essentially it boiled down
  to checking if the index was used
* the metric value incorrectly recorded queries with no skips, because
  the histogram was configured to not allow zeroes.

In addition, the usage of index is dependent on the version of the
optimizer, so that made the test behave differently between DC and EC
index version. Now that we disable the optimizer for this test,
that difference is gone.

Fixes riptano/cndb#13830

---------

Co-authored-by: ekaterinadimitrova2 <[email protected]>

Author: pkolaczk

src/java/org/apache/cassandra/config/CassandraRelevantProperties.java

@@ -369,6 +369,9 @@ public enum CassandraRelevantProperties
     /** Whether to validate terms that will be SAI indexed at the coordinator */
     SAI_VALIDATE_TERMS_AT_COORDINATOR("cassandra.sai.validate_terms_at_coordinator", "true"),
 
+    /** Whether to optimize query plans */
+    SAI_QUERY_OPTIMIZATION_LEVEL("cassandra.sai.query_optimization_level", "1"),
+
     /** Whether vector type only allows float vectors. True by default. **/
     VECTOR_FLOAT_ONLY("cassandra.float_only_vectors", "true"),
     /** Enables use of vector type. True by default. **/

src/java/org/apache/cassandra/index/sai/metrics/TableQueryMetrics.java

@@ -120,12 +120,12 @@ public PerQueryMetrics(TableMetadata table)
             sstablesHit = Metrics.histogram(createMetricName("SSTableIndexesHit"), false);
             segmentsHit = Metrics.histogram(createMetricName("IndexSegmentsHit"), false);
 
-            kdTreePostingsSkips = Metrics.histogram(createMetricName("KDTreePostingsSkips"), false);
+            kdTreePostingsSkips = Metrics.histogram(createMetricName("KDTreePostingsSkips"), true);
 
             kdTreePostingsNumPostings = Metrics.histogram(createMetricName("KDTreePostingsNumPostings"), false);
             kdTreePostingsDecodes = Metrics.histogram(createMetricName("KDTreePostingsDecodes"), false);
 
-            postingsSkips = Metrics.histogram(createMetricName("PostingsSkips"), false);
+            postingsSkips = Metrics.histogram(createMetricName("PostingsSkips"), true);
             postingsDecodes = Metrics.histogram(createMetricName("PostingsDecodes"), false);
 
             partitionReads = Metrics.histogram(createMetricName("PartitionReads"), false);

src/java/org/apache/cassandra/index/sai/plan/QueryController.java

@@ -27,6 +27,7 @@
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
+import org.apache.cassandra.config.CassandraRelevantProperties;
 import org.apache.cassandra.index.FeatureNeedsIndexRebuildException;
 import org.apache.cassandra.index.sai.disk.format.Version;
 import org.slf4j.Logger;
@@ -106,7 +107,7 @@ public class QueryController implements Plan.Executor, Plan.CostEstimator
      * Note: the config is not final to simplify testing.
      */
     @VisibleForTesting
-    public static int QUERY_OPT_LEVEL = Integer.getInteger("cassandra.sai.query.optimization.level", 1);
+    public static int QUERY_OPT_LEVEL = CassandraRelevantProperties.SAI_QUERY_OPTIMIZATION_LEVEL.getInt();
 
     private final ColumnFamilyStore cfs;
     private final ReadCommand command;

test/unit/org/apache/cassandra/index/sai/metrics/AbstractMetricsTest.java

@@ -56,7 +56,7 @@ protected void waitForIndexCompaction(String keyspace, String table, String inde
         }, 60, TimeUnit.SECONDS);
     }
 
-    protected void waitForVerifyHistogram(ObjectName name, long count)
+    protected void waitForHistogramCountEquals(ObjectName name, long count)
     {
         waitForAssert(() -> {
             try
@@ -70,6 +70,22 @@ protected void waitForVerifyHistogram(ObjectName name, long count)
         }, 10, TimeUnit.SECONDS);
     }
 
+    protected void waitForHistogramMeanBetween(ObjectName name, double min, double max)
+    {
+        waitForAssert(() -> {
+            try
+            {
+                double mean = (double) jmxConnection.getAttribute(name, "Mean");
+                assertTrue("Median " + mean + " is between " + min + " and " + max, mean >= min && mean <= max);
+            }
+            catch (Throwable ex)
+            {
+                throw Throwables.unchecked(ex);
+            }
+        }, 10, TimeUnit.SECONDS);
+    }
+
+
     protected void waitForGreaterThanZero(ObjectName name)
     {
         waitForAssert(() -> {

test/unit/org/apache/cassandra/index/sai/metrics/IndexMetricsTest.java

@@ -110,7 +110,7 @@ public void testMetricsThroughWriteLifecycle()
         assertEquals(0L, getMetricValue(objectName("DiskUsedBytes", KEYSPACE, table, index, "IndexMetrics")));
         assertEquals(0L, getMetricValue(objectName("CompactionCount", KEYSPACE, table, index, "IndexMetrics")));
 
-        waitForVerifyHistogram(objectName("MemtableIndexFlushCellsPerSecond", KEYSPACE, table, index, "IndexMetrics"), 0);
+        waitForHistogramCountEquals(objectName("MemtableIndexFlushCellsPerSecond", KEYSPACE, table, index, "IndexMetrics"), 0);
 
         flush(KEYSPACE, table);
 
@@ -122,7 +122,8 @@ public void testMetricsThroughWriteLifecycle()
         assertTrue((Long)getMetricValue(objectName("DiskUsedBytes", KEYSPACE, table, index, "IndexMetrics")) > 0);
         assertEquals(0L, getMetricValue(objectName("CompactionCount", KEYSPACE, table, index, "IndexMetrics")));
 
-        waitForVerifyHistogram(objectName("MemtableIndexFlushCellsPerSecond", KEYSPACE, table, index, "IndexMetrics"), 1);
+        waitForHistogramCountEquals(objectName("MemtableIndexFlushCellsPerSecond", KEYSPACE, table, index, "IndexMetrics"), 1);
+        waitForHistogramMeanBetween(objectName("MemtableIndexFlushCellsPerSecond", KEYSPACE, table, index, "IndexMetrics"), 1.0, 1000000.0);
 
         compact(KEYSPACE, table);
 
@@ -139,7 +140,8 @@ public void testMetricsThroughWriteLifecycle()
         assertTrue((Long)getMetricValue(objectName("DiskUsedBytes", KEYSPACE, table, index, "IndexMetrics")) > 0);
         assertEquals(1L, getMetricValue(objectName("CompactionCount", KEYSPACE, table, index, "IndexMetrics")));
 
-        waitForVerifyHistogram(objectName("CompactionSegmentCellsPerSecond", KEYSPACE, table, index, "IndexMetrics"), 1);
+        waitForHistogramCountEquals(objectName("CompactionSegmentCellsPerSecond", KEYSPACE, table, index, "IndexMetrics"), 1);
+        waitForHistogramMeanBetween(objectName("CompactionSegmentCellsPerSecond", KEYSPACE, table, index, "IndexMetrics"), 1.0, 1000000.0);
     }
 
     private void assertIndexQueryCount(String index, long expectedCount)

test/unit/org/apache/cassandra/index/sai/metrics/QueryMetricsTest.java

@@ -28,6 +28,7 @@
 
 import com.datastax.driver.core.ResultSet;
 import org.apache.cassandra.index.sai.disk.format.Version;
+import org.apache.cassandra.index.sai.plan.QueryController;
 import org.apache.cassandra.metrics.CassandraMetricsRegistry;
 
 import static org.apache.cassandra.index.sai.metrics.TableQueryMetrics.TABLE_QUERY_METRIC_TYPE;
@@ -225,6 +226,11 @@ public void testKDTreeQueryMetricsWithSingleIndex()
     @Test
     public void testKDTreePostingsQueryMetricsWithSingleIndex()
     {
+        // Turn off the query optimizer.
+        // We need to do this in order to remove unpredictability of query plans, so that we get consistent metrics.
+        // We don't want the query optimizer to eliminate the use of indexes.
+        QueryController.QUERY_OPT_LEVEL = 0;
+
         String table = "test_kdtree_postings_metrics_through_write_lifecycle";
         String v1Index = "test_kdtree_postings_metrics_through_write_lifecycle_v1_index";
         String v2Index = "test_kdtree_postings_metrics_through_write_lifecycle_v2_index";
@@ -255,13 +261,20 @@ public void testKDTreePostingsQueryMetricsWithSingleIndex()
         assertEquals(rowsWritten, actualRows);
 
         assertTrue(((Number) getMetricValue(objectName("NumPostings", keyspace, table, v1Index, "KDTreePostings"))).longValue() > 0);
+        waitForHistogramCountEquals(objectNameNoIndex("KDTreePostingsNumPostings", keyspace, table, PER_QUERY_METRIC_TYPE), 1);
+        waitForHistogramMeanBetween(objectNameNoIndex("KDTreePostingsNumPostings", keyspace, table, PER_QUERY_METRIC_TYPE), 1.0, 1.0);
 
-        waitForVerifyHistogram(objectNameNoIndex("KDTreePostingsNumPostings", keyspace, table, PER_QUERY_METRIC_TYPE), 1);
+        // the query performed no skips, but the metric should be updated because the index was used, so we should get
+        // a single entry in the histogram with 0 skips
+        waitForHistogramCountEquals(objectNameNoIndex("KDTreePostingsSkips", keyspace, table, PER_QUERY_METRIC_TYPE), 1);
+        waitForHistogramMeanBetween(objectNameNoIndex("KDTreePostingsSkips", keyspace, table, PER_QUERY_METRIC_TYPE), 0.0, 0.0);
 
-        // V2 index is very selective, so it should lead the union merge process, causing V1 index to be not used at all.
-        execute("SELECT id1 FROM " + keyspace + "." + table + " WHERE v1 >= 0 AND v1 <= 1000 AND v2 = '5' ALLOW FILTERING");
+        // V2 index is very selective, so it should lead the union merge process, causing V1 index to skip/advance
+        execute("SELECT id1 FROM " + keyspace + "." + table + " WHERE v1 >= 0 AND v1 <= 1000 AND v2 IN ('5', '10', '20', '22') ALLOW FILTERING");
 
-        waitForVerifyHistogram(objectNameNoIndex("KDTreePostingsSkips", keyspace, table, PER_QUERY_METRIC_TYPE), 2);
+        // we expect exactly 4 skips from this query, but the mean will be 2.0 because of the previous query which had 0 skips
+        waitForHistogramCountEquals(objectNameNoIndex("KDTreePostingsSkips", keyspace, table, PER_QUERY_METRIC_TYPE), 2);
+        waitForHistogramMeanBetween(objectNameNoIndex("KDTreePostingsSkips", keyspace, table, PER_QUERY_METRIC_TYPE), 1.99, 2.01);
     }
 
     @Test
@@ -353,7 +366,7 @@ public void testKDTreePartitionsReadAndRowsFiltered()
 
         //TODO This needs revisiting with STAR-903 because we are now reading rows one at a time
         waitForEquals(objectNameNoIndex("TotalPartitionReads", keyspace, table, TABLE_QUERY_METRIC_TYPE), Version.current() == Version.AA ? 2 : 3);
-        waitForVerifyHistogram(objectNameNoIndex("RowsFiltered", keyspace, table, PER_QUERY_METRIC_TYPE), 1);
+        waitForHistogramCountEquals(objectNameNoIndex("RowsFiltered", keyspace, table, PER_QUERY_METRIC_TYPE), 1);
         waitForEquals(objectNameNoIndex("TotalRowsFiltered", keyspace, table, TABLE_QUERY_METRIC_TYPE), 3);
     }