Skip to content

Commit

Permalink
add testMatchingAllowed and make it work via shouldMerge
Browse files Browse the repository at this point in the history
  • Loading branch information
jbellis committed Dec 11, 2024
1 parent cef71e3 commit 3d17e2f
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 12 deletions.
25 changes: 13 additions & 12 deletions src/java/org/apache/cassandra/cql3/restrictions/RestrictionSet.java
Original file line number Diff line number Diff line change
Expand Up @@ -427,21 +427,22 @@ public void addRestriction(SingleRestriction restriction, boolean isDisjunction)
{
// ANDed together restrictions against the same columns should be merged.
Set<SingleRestriction> existingRestrictions = getRestrictions(newRestrictions, columnDefs);
// Trivial case of no existing restrictions
if (existingRestrictions.isEmpty())

// merge the new restriction into an existing one. note that there is only ever a single
// restriction (per column), UNLESS one is ORDER BY BM25 and the other is MATCH.
for (var existing : existingRestrictions)
{
addRestrictionForColumns(columnDefs, restriction, null);
return;
// shouldMerge exists for the BM25/MATCH case
if (existing.shouldMerge(restriction))
{
var merged = existing.mergeWith(restriction);
addRestrictionForColumns(merged.getColumnDefs(), merged, Set.of(existing));
return;
}
}
// Since we merge new restrictions into the existing ones at each pass, there should only be
// at most one existing restriction across the same columnDefs
assert existingRestrictions.size() == 1 : existingRestrictions;

// Perform the merge
SingleRestriction existing = existingRestrictions.iterator().next();
var merged = existing.mergeWith(restriction);

addRestrictionForColumns(merged.getColumnDefs(), merged, Set.of(existing));
// no existing restrictions that we should merge the new one with, add a new one
addRestrictionForColumns(columnDefs, restriction, null);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1237,6 +1237,17 @@ public boolean isIndexBasedOrdering()
{
return true;
}

@Override
public boolean shouldMerge(SingleRestriction other)
{
// we don't want to merge MATCH restrictions with ORDER BY BM25
// so shouldMerge = false for that scenario, and true for others
// (because even though we can't meaningfully merge with others, we want doMergeWith to be called to throw)
//
// (Note that because ORDER BY is processed before WHERE, we only need this check in the BM25 class)
return !other.isAnalyzerMatches();
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,4 +129,16 @@ public default MultiClusteringBuilder appendBoundTo(MultiClusteringBuilder build
{
return appendTo(builder, options);
}

/**
* @return true if the other restriction should be merged with this one.
* This is NOT for preventing illegal combinations of restrictions, e.g.
* a=1 AND a=2; that is handled by mergeWith. Instead, this is for the case
* where we want two completely different semantics against the same column.
* Currently the only such case is BM25 with MATCH.
*/
default boolean shouldMerge(SingleRestriction other)
{
return true;
}
}
16 changes: 16 additions & 0 deletions test/unit/org/apache/cassandra/index/sai/cql/BM25Test.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,22 @@

public class BM25Test extends SAITester
{
@Test
public void testMatchingAllowed() throws Throwable
{
// match operator should be allowed with BM25 on the same column
// (seems obvious but exercises a corner case in the internal RestrictionSet processing)
createSimpleTable();

execute("INSERT INTO %s (k, v) VALUES (1, 'apple')");

beforeAndAfterFlush(() ->
{
var result = execute("SELECT k FROM %s WHERE v : 'apple' ORDER BY v BM25 OF 'apple' LIMIT 3");
assertRows(result, row(1));
});
}

@Test
public void testTermFrequencyOrdering() throws Throwable
{
Expand Down

0 comments on commit 3d17e2f

Please sign in to comment.