dcapwell commented on code in PR #3779:
URL: https://github.com/apache/cassandra/pull/3779#discussion_r1932892471


##########
test/distributed/org/apache/cassandra/fuzz/sai/SingleNodeSAITestBase.java:
##########
@@ -136,17 +164,20 @@ private void basicSaiTest(EntropySource rng, SchemaSpec 
schema)
         cluster.forEach(i -> i.nodetool("disableautocompaction"));
 
         cluster.schemaChange(schema.compile());
-        cluster.schemaChange(schema.compile().replace(schema.keyspace + "." + 
schema.table,
-                                                      schema.keyspace + 
".debug_table"));
-        Streams.concat(schema.clusteringKeys.stream(),
-                       schema.regularColumns.stream(),
-                       schema.staticColumns.stream())
+        cluster.schemaChange(schema.compile().replace(schema.keyspace + '.' + 
schema.table, schema.keyspace + ".debug_table"));
+
+        Streams.concat(schema.clusteringKeys.stream(), 
schema.regularColumns.stream(), schema.staticColumns.stream())
                .forEach(column -> {
+                   if (createIndex.get())

Review Comment:
   turns out you do `() -> rng.nextFloat() < 0.7f`.  So this idea isn't too 
crazy =D



##########
test/distributed/org/apache/cassandra/fuzz/sai/SingleNodeSAITestBase.java:
##########
@@ -104,28 +111,49 @@ public static void afterClass()
     @Before
     public void beforeEach()
     {
-        cluster.schemaChange("DROP KEYSPACE IF EXISTS harry");
-        cluster.schemaChange("CREATE KEYSPACE harry WITH replication = 
{'class': 'SimpleStrategy', 'replication_factor': 1};");
+        cluster.schemaChange("DROP KEYSPACE IF EXISTS " + KEYSPACE);
+        cluster.schemaChange("CREATE KEYSPACE " + KEYSPACE + " WITH 
replication = {'class': 'SimpleStrategy', 'replication_factor': 1};");
     }
 
     @Test
     public void simplifiedSaiTest()
     {
-        withRandom(rng -> basicSaiTest(rng, 
SchemaGenerators.trivialSchema("harry", "simplified", 1000).generate(rng)));
+        withRandom(rng -> saiTest(rng,
+                                  SchemaGenerators.trivialSchema(KEYSPACE, 
"simplified", 1000).generate(rng),
+                                  () -> true,
+                                  DEFAULT_REPAIR_SKIP));
+    }
+
+    @Test
+    public void indexOnlySaiTest()
+    {
+        Generator<SchemaSpec> schemaGen = schemaGenerator(false);
+        withRandom(rng -> saiTest(rng, schemaGen.generate(rng), () -> true, 
DEFAULT_REPAIR_SKIP));
+    }
+
+    @Test
+    public void indexOnlyNoRepairSaiTest()
+    {
+        Generator<SchemaSpec> schemaGen = schemaGenerator(true);
+        withRandom(rng -> saiTest(rng, schemaGen.generate(rng), () -> true, 
Integer.MAX_VALUE));

Review Comment:
   correct me if i am wrong, but shouldn't this be 4 tests, not 2?
   
   ```
   disable read_repair + no repair
   enable read_repair + with repair



##########
test/distributed/org/apache/cassandra/fuzz/sai/SingleNodeSAITestBase.java:
##########
@@ -156,20 +187,28 @@ private void basicSaiTest(EntropySource rng, SchemaSpec 
schema)
                                                                                
             .pageSizeSelector(pageSizeSelector(rng))
                                                                                
             .consistencyLevel(consistencyLevelSelector())
                                                                                
             .doubleWriting(schema, hb, cluster, "debug_table"));
-        List<Integer> partitions = new ArrayList<>();
-        for (int j = 0; j < 5; j++)
-        {
-            int picked = globalPkGen.generate(rng);
-            if (usedPartitions.contains(picked))
-                continue;
-            partitions.add(picked);
-        }
+        Set<Integer> partitions = new HashSet<>();
+        while (partitions.size() < NUM_VISITED_PARTITIONS)

Review Comment:
   checked and the types supported are listed below, and you can have 1+ columns
   
   ```
   public static final List<DataType<?>> TYPES;
   
       static
       {
           List<DataType<?>> types = new ArrayList<>()
           {{
               add(int8Type);
               add(int16Type);
               add(int32Type);
               add(int64Type);
               add(floatType);
               add(doubleType);
               // TODO: SAI tests seem to fail these types
               // add(booleanType);
               // add(inetType);
               // add(varintType);
               // add(decimalType);
               add(asciiType);
               add(textType);
               // TODO: blob is not supported in SAI
               // add(blobType);
               add(uuidType);
               add(timestampType);
               // TODO: SAI test fails due to TimeSerializer#toString in tracing
               //  add(timeType);
               // TODO: compose proper value
               // add(timeUuidType);
           }};
           TYPES = Collections.unmodifiableList(types);
       }
   ```
   
   `booleanType` is the only type that should break this logic...
   
   You might want to take a look at `accord.utils.Gens.GenReset#next`. there is 
a concept of `bestEffort` when trying to produce unique values specifically to 
handle this case.... if the pk is `booleanType` and you have less than 3 
columns, this will loop forever



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to