Cep 29 not operator

[pkolaczk]

• NOT IN
• NOT CONTAINS / NOT CONTAINS KEY
• Inequality operator (!=) for map entries
• Inequality operator (!=) for clustering columns
• Both ALLOW FILTERING and indexing (SAI) supported

[adelapena]

The implementations of SingleColumnRestriction.SliceRestriction#isSupportedBy and MultiColumnRestriction.SliceRestriction#isSupportedBy are identical. Also, both implementations are the only callers of TermSlice#isSupportedBy(ColumnMetadata, Index).

Maybe we could pass the list of skipped values to TermSlice#isSupportedBy and delegate the NEQ check to it, putting similar things together and preventing some duplication:

public boolean isSupportedBy(ColumnMetadata column, Index index, List<MarkerOrTerms> skippedValues)
{
    boolean supported = false;

    if (hasBound(Bound.START))
        supported |= isInclusive(Bound.START) ? index.supportsExpression(column, Operator.GTE)
                : index.supportsExpression(column, Operator.GT);
    if (hasBound(Bound.END))
        supported |= isInclusive(Bound.END) ? index.supportsExpression(column, Operator.LTE)
                : index.supportsExpression(column, Operator.LT);

    supported |= !skippedValues.isEmpty() && index.supportsExpression(column, Operator.NEQ);

    return supported;
}

[pkolaczk]

I don't agree on this one. This is just coincidental duplication - accidentally both single and multi restrictions work the same way here, but there is no fundamental reason they have to. The amount of duplication is also very tiny. And introducing an unrelated parameter to TermSlice is mixing responsibilities which is IMHO worse.

I thought that maybe we could move the skipped values to the TermSlice itself. Then it would make more sense to have TermSlice handle the isSupportedBy logic as it would have all the information it needs. WDYT?

[adelapena]

Agree, moving the skipped values to the TermSlice sounds good to me.

[adelapena]

@pkolaczk there is a bunch of unresolved nits above that only show up if you click on the 12 hidden conversations Load more... link on the GH web UI, maybe you have missed them?

[adelapena]

I have noticed that this test returns more rows than expected:

@Test
public void notInTest()
{
    createTable("CREATE TABLE %s (k int PRIMARY KEY, v1 int, v2 int)");
    createIndex("CREATE INDEX ON %s(v1) USING 'sai'");
    createIndex("CREATE INDEX ON %s(v2) USING 'sai'");

    execute("INSERT INTO %s (k, v1, v2) VALUES (1, 1, 4)");
    execute("INSERT INTO %s (k, v1, v2) VALUES (2, 2, 3)");
    execute("INSERT INTO %s (k, v1, v2) VALUES (3, 3, 2)");
    execute("INSERT INTO %s (k, v1, v2) VALUES (4, 4, 1)");

    waitForTableIndexesQueryable(currentTable());

    assertRowsIgnoringOrder(execute("SELECT * FROM %s"),
            row(1, 1, 4),
            row(2, 2, 3),
            row(3, 3, 2),
            row(4, 4, 1));

    assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE v1 NOT IN (1)"),
            row(2, 2, 3),
            row(3, 3, 2),
            row(4, 4, 1));

    assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE v1 NOT IN (4)"),
            row(1, 1, 4),
            row(2, 2, 3),
            row(3, 3, 2));

    assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE v1 NOT IN (1, 4)"),
            row(2, 2, 3),
            row(3, 3, 2)); // Got 1 extra row(s) in result: (k=1, v1=1, v2=4)

    assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE v1 NOT IN (1, 4) AND v2 NOT IN (3, 2)")); // 2 extra rows
}

However, the test passes if we don't create the indexes and use ALLOW FILTERING instead:

@Test
public void notInTest()
{
    createTable("CREATE TABLE %s (k int PRIMARY KEY, v1 int, v2 int)");

    execute("INSERT INTO %s (k, v1, v2) VALUES (1, 1, 4)");
    execute("INSERT INTO %s (k, v1, v2) VALUES (2, 2, 3)");
    execute("INSERT INTO %s (k, v1, v2) VALUES (3, 3, 2)");
    execute("INSERT INTO %s (k, v1, v2) VALUES (4, 4, 1)");

    assertRowsIgnoringOrder(execute("SELECT * FROM %s"),
            row(1, 1, 4),
            row(2, 2, 3),
            row(3, 3, 2),
            row(4, 4, 1));

    assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE v1 NOT IN (1) ALLOW FILTERING"),
            row(2, 2, 3),
            row(3, 3, 2),
            row(4, 4, 1));

    assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE v1 NOT IN (4) ALLOW FILTERING"),
            row(1, 1, 4),
            row(2, 2, 3),
            row(3, 3, 2));

    assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE v1 NOT IN (1, 4) ALLOW FILTERING"),
            row(2, 2, 3),
            row(3, 3, 2));

    assertRowsIgnoringOrder(execute("SELECT * FROM %s WHERE v1 NOT IN (1, 4) AND v2 NOT IN (3, 2) ALLOW FILTERING"));
}

[pkolaczk]

there is a bunch of unresolved nits above that only show up if you click on the 12 hidden conversations Load more... link on the GH web UI, maybe you have missed them?

lol, indeed, I missed them.

[pkolaczk]

I reverted the support for != and NOT IN on indexed columns for now as current index search does not allow multiple exclusions.

[adelapena]

Albeit the reversal of the support for != and NOT IN on indexed columns has made it necessary to use ALLOW FILTERING again, it seems that the indexes are still involved in the query and producing wrong results:

@Test
public void notInTest()
{
    createTable("CREATE TABLE %s (k int PRIMARY KEY, v int)");

    execute("INSERT INTO %s (k, v) VALUES (1, 1)");
    execute("INSERT INTO %s (k, v) VALUES (2, 2)");
    execute("INSERT INTO %s (k, v) VALUES (3, 3)");
    execute("INSERT INTO %s (k, v) VALUES (4, 4)");

    String query = "SELECT * FROM %s WHERE v NOT IN (1, 4) ALLOW FILTERING";
    Object[][] rows = rows(row(2, 2), row(3, 3));

    assertRowsIgnoringOrder(execute(query), rows);

    createIndex("CREATE INDEX ON %s(v) USING 'sai'");
    waitForTableIndexesQueryable();

    assertRowsIgnoringOrder(execute(query), rows); // Got 1 extra row(s) in result: (k=1, v1=1)
}

I suggest we add some additional tests around the use of unsupported operators with SAI indexes and ALLOW FILTERING.

[Mmuzaf]

Hi, I prefer to make these sorts of changes in a dedicated issue, so it looks like we can significantly reduce the number of changed files if we revert it, and keep only necessary changes here.

See the discussion below, we can ignore code formatting changes by listing the relevant commit hashes in a special file (this is not used by the project right now). So if we do this change in another issue and put that commit hash in a special file, it won't pollute the git blame in our IDEs.
https://github.com/orgs/community/discussions/5033
https://git-scm.com/docs/git-blame#Documentation/git-blame.txt-blameignoreRevsFile

This is my humble argument for such not making such a change here :-)

[Mmuzaf]

The iterator looks very similar to this one, do we still need to keep it?

    private SSTableRowIdKeyRangeIterator(PrimaryKey min,
                                         PrimaryKey max,
                                         long count,
                                         PrimaryKeyMap primaryKeyMap,
                                         QueryContext queryContext,
                                         PostingList postingList)