Add basic text analysis to SAI:

[mike-tr-adamson]

• Adds the case_sensitive, normalize and ascii options to the index.

[adelapena]

Since CASSANDRA-18521 CQLTester#waitForIndexQueryable requires an argument specifying the index name. Its absence here produces a build error. Maybe it has been missed during rebase?

[adelapena]

Note that also CQLTester#waitForTableIndexesQueryable can be used to just wait for all the indexes used by the table.

[mike-tr-adamson]

Since waitForTableIndexesQueryable was added to createIndex, I have removed these calls.

[adelapena]

Maybe it's simpler to get rid of ANALYZABLE_TYPES and TypeUtil#isIn and just use (type instanceof StringType) to check if a type is analyzable?

[adelapena]

Nit: can be protected

[adelapena]

Nit: add @Override

[adelapena]

I think this @Override is missed in the last commit

[mike-tr-adamson]

Sorry, not sure how I missed that one.

[adelapena]

Nit: add @Override

[adelapena]

Nit: add @Override

[adelapena]

Nit: add @Override

[adelapena]

Nit: no need to break the line

[adelapena]

It seems there is some code duplication across the tests in this class. Maybe we could use a utility method such as, for example:

private void test(String input, String expected, AbstractAnalyzer analyzer) throws Exception
{
    ByteBuffer toAnalyze = ByteBuffer.wrap(input.getBytes());
    analyzer.reset(toAnalyze);
    ByteBuffer analyzed = null;
    while (analyzer.hasNext())
    {
        analyzed = analyzer.next();
    }
    String result = ByteBufferUtil.string(analyzed);
    assertEquals(expected, result);
}

[mike-tr-adamson]

Agreed, I've done this and generally simplified / tidied this test.

[adelapena]

It looks nice now :)

[mike-tr-adamson]

@adelapena @bereng Please note that I have just made a number of changes for the Lucene 9.7 upgrade.

[adelapena]

Maybe assertEquals("Nip it in the bud", getAnalyzedString("Nip it in the bud", options)); achieves the same and is more strict by verifying what is exactly returned?

[mike-tr-adamson]

Agreed, I'm not entirely sure why the original test did this because it would have failed on any string.

[adelapena]

@mike-tr-adamson I think it would be good to add a dtest quickly verifying that tokenisation and analysis don't break RFP (classic CASSANDRA-8272). I think something like this would work:

public class ReplicaFilteringProtectionTest extends TestBaseImpl
{
    private static final int REPLICAS = 2;

    @Test
    public void testRFPWithIndexTransformations() throws IOException
    {
        try (Cluster cluster = init(Cluster.build()
                                           .withNodes(REPLICAS)
                                           .withConfig(config -> config.set("hinted_handoff_enabled", false)
                                                                       .set("commitlog_sync", "batch")).start()))
        {
            String tableName = "sai_rfp";
            String fullTableName = KEYSPACE + '.' + tableName;

            cluster.schemaChange("CREATE TABLE " + fullTableName + " (k int PRIMARY KEY, v text)");
            cluster.schemaChange("CREATE CUSTOM INDEX ON " + fullTableName + "(v) USING 'StorageAttachedIndex' " +
                                 "WITH OPTIONS = { 'case_sensitive' : false}");

            // both nodes have the old value
            cluster.coordinator(1).execute("INSERT INTO " + fullTableName + "(k, v) VALUES (0, 'OLD')", ALL);

            String select = "SELECT * FROM " + fullTableName + " WHERE v = 'old'";
            Object[][] initialRows = cluster.coordinator(1).execute(select, ALL);
            assertRows(initialRows, row(0, "OLD"));

            // only one node gets the new value
            cluster.get(1).executeInternal("UPDATE " + fullTableName + " SET v = 'new' WHERE k = 0");

            // querying by the old value shouldn't return the old surviving row
            SimpleQueryResult oldResult = cluster.coordinator(1).executeWithResult(select, ALL);
            assertRows(oldResult.toObjectArrays());
        }
    }
}

[mike-tr-adamson]

@adelapena I've added the ReplicaFilteringProtectionTest but, I have to admit, I'm not entirely sure how/why it's working. My main concern is that the test is passing for reasons that aren't the ones that we are testing for. I will have a bit of a dig to confirm this.

[adelapena]

@mike-tr-adamson I think RFP is working because StorageAttachedIndexSearcher #filterReplicaFilteringProtection takes care of applying the filters to the expressions that are used in the coordinator:

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

Lines 85 to 103 in 723cc16

	public PartitionIterator filterReplicaFilteringProtection(PartitionIterator fullResponse)
	{
	for (RowFilter.Expression expression : queryController.filterOperation())
	{
	AbstractAnalyzer analyzer = queryController.getContext(expression).getAnalyzerFactory().create();
	try
	{
	if (analyzer.transformValue())
	return applyIndexFilter(fullResponse, Operation.buildFilter(queryController), queryContext);
	}
	finally
	{
	analyzer.end();
	}
	}
	// if no analyzer does transformation
	return Index.Searcher.super.filterReplicaFilteringProtection(fullResponse);
	}

You can artificially see the test failing if you modify that method or if you make DataResolver#needsReplicaFilteringProtection return false.

[mike-tr-adamson]

Thank you, that's what I was trying to find.

[adelapena]

This unused import is breaking the build: https://app.circleci.com/pipelines/github/adelapena/cassandra/3004/workflows/cff4d9ec-aa49-466d-9f32-8535c3826a64/jobs/56610

[mike-tr-adamson]

My bad, sorry. I'll get my commit process correct one of these days.

[maedhroz]

Committed as 05dd587