Implement ORDER BY BM25 #1434

jbellis · 2024-11-20T20:43:18Z

What is the issue

https://github.com/riptano/cndb/issues/11725

What does this PR fix and why was it fixed

...

Checklist before you submit for review

Make sure there is a PR in the CNDB project updating the Converged Cassandra version
Use NoSpamLogger for log lines that may appear frequently in the logs
Verify test results on Butler
Test coverage for new/modified code is > 80%
Proper code formatting
Proper title for each commit staring with the project-issue number, like CNDB-1234
Each commit has a meaningful description
Each commit is not very long and contains related changes
Renames, moves and reformatting are in distinct commits

jbellis · 2024-11-22T19:34:33Z

(force push is identical code that CI ran previously, just cleaned up the history)

michaeljmarshall

Looks great! I'm really happy with how much this cleaned up the index based ordering logic in several classes.

Left a handful of minor comments/questions.

src/java/org/apache/cassandra/cql3/restrictions/StatementRestrictions.java

test/unit/org/apache/cassandra/index/sai/StorageAttachedIndexTest.java

src/java/org/apache/cassandra/index/sai/QueryContext.java

src/java/org/apache/cassandra/schema/ColumnMetadata.java

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

src/java/org/apache/cassandra/db/filter/ColumnFilter.java

src/java/org/apache/cassandra/db/Columns.java

src/java/org/apache/cassandra/cql3/statements/SelectStatement.java

…onsible for ANN index queries. Other global orderings will be represented by a SingleColumnComparator with clustered=true instead.

… of recomputing scores on the coordinator

…olumn

michaeljmarshall

Nice work! I left several minor comments and a few larger questions.

src/java/org/apache/cassandra/cql3/GeoDistanceRelation.java

src/java/org/apache/cassandra/cql3/Operator.java

src/java/org/apache/cassandra/index/sai/disk/v1/postings/IntersectingPostingList.java

src/java/org/apache/cassandra/index/sai/utils/BM25Utils.java

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

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

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

src/java/org/apache/cassandra/index/sai/disk/v1/InvertedIndexSearcher.java

…ction in SingleColumnRelation.newEQRestriction. This eliminates the need for skipMerge and special cases in doMergeWith, and moves the issuing of warnings next to the place where the transformation occurs instead of doing it much later in RowFilterValidator (which is no longer needed)

…testTwoIndexes passes

…tes passes

… approach than ignoring it when serialization fails later

jbellis · 2025-02-14T22:21:12Z

I think the GenericOrderByTest failure is real :(

michaeljmarshall · 2025-02-19T20:20:38Z

@jbellis looks like we're duplicating the sort value so that the rows have 4 columns instead of 3. When I update the order by test to use k, c, v instead of * to select the right columns, it passes some of the assertions and fails with a new error:

java.lang.AssertionError: Expected: [[3, 3, 9],[1, 2, 8],[3, 1, 7],[2, 1, 6],[2, 2, 5],[2, 3, 4]]
Actual: [[3, 3, 9],[1, 2, 8],[1, 2, 8],[3, 1, 7],[2, 1, 6],[2, 2, 5]]

That error looks like we're not deduping rows properly.

…ostly do different things, jamming the logic together in "processPartitions" caused way more complexity than saving a repeated while loop was worth

michaeljmarshall · 2025-02-24T15:32:58Z

IndexMetricsTest::testQueriesCount fails locally on my machine

…ented

jbellis · 2025-02-25T13:50:01Z

Something wonky with Butler?

BinLogTest: looks flaky, nothing new here
testOneToManyCompaction: blank entry in the Butler report
rangeRestrictedTest: no entry at all in the report

jbellis · 2025-02-25T13:50:24Z

VectorLocalTest and VectorCompactionTest both pass locally

michaeljmarshall · 2025-02-25T21:02:47Z

src/java/org/apache/cassandra/cql3/statements/SelectStatement.java

+                // by synthetic +score column.
+                boolean cqlReversed = ordering.direction == Ordering.Direction.DESC;
+                if (def.position() == ColumnMetadata.NO_POSITION)
+                    return ordering.expression.isScored() || cqlReversed;


I don't think this hurts anything as is, but is the "isScored()" ever going to return true?

Ann and BM25 Ordering subclasses override isScored() to true. Maybe I'm not understanding your point correctly.

michaeljmarshall

LGTM! Nice work :) just waiting on CI now

… so code around that

sonarqubecloud · 2025-02-27T01:04:21Z

Quality Gate passed

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
91.9% Coverage on New Code
0.5% Duplication on New Code

See analysis details on SonarQube Cloud

cassci-bot · 2025-02-27T01:24:39Z

✔️ Build ds-cassandra-pr-gate/PR-1434 approved by Butler

Approved by Butler
See build details here

### What is the issue The `NON_DAEMON` IndexRegistry returns `true` for all calls to `supportsExpression`. This leads to an incorrect result from the `getEqBehavior` method where we get `MATCH` instead of `EQ` because the index indicates that it can handle `ANALYZER_MATCHES` expressions and `EQ` expressions. It is an odd edge case because the javadoc for the `NON_DAEMON` object is: ```java /** * An {@code IndexRegistry} intended for use when Cassandra is initialized in client or tool mode. * Contains a single stub {@code Index} which possesses no actual indexing or searching capabilities * but enables query validation and preparation to succeed. Useful for tools which need to prepare * CQL statements without instantiating the whole ColumnFamilyStore infrastructure. */ ``` This presents a problem for the eq/match logic where we want to find a nuanced solution to the different solutions. My proposal here is to just make it use the EQ behavior, but that might have adverse side effects in untested code. ### What does this PR fix and why was it fixed The original fix used in #1434 was just to avoid the NPE we hit, but it allowed for the wrong result in eq behavior. This fix is to say that we should just return `EQ` in that case. It's possible this fix has negative consequences that we haven't seen yet, but at the very least, the CNDB tests pass with it.

jbellis force-pushed the scoreordering-6 branch from 3a50985 to 3848e94 Compare November 22, 2024 19:34

michaeljmarshall approved these changes Nov 22, 2024

View reviewed changes

jbellis force-pushed the scoreordering-6 branch from 03036e4 to a5060e9 Compare November 25, 2024 15:39

jbellis added 2 commits November 25, 2024 10:25

remove unnecessary generification of IndexColumnComparator

ddd9b69

Simplify the ordering logic by making IndexColumnComparator only resp…

9f1b794

…onsible for ANN index queries. Other global orderings will be represented by a SingleColumnComparator with clustered=true instead.

jbellis force-pushed the scoreordering-6 branch from a5060e9 to 5776439 Compare November 25, 2024 16:25

jbellis added 2 commits November 26, 2024 09:21

CNDB-11725 use +score pseudo-column to order ANN results with instead…

9986108

… of recomputing scores on the coordinator

CNDB-11725 add SYNTHETIC ColumnMetadata.Kind to represent the score c…

e0ea872

…olumn

jbellis force-pushed the scoreordering-6 branch from 5776439 to e0ea872 Compare November 26, 2024 15:32

merge with main

478ca65

jbellis changed the title ~~Send similarity score from writer to coordinator for faster sorting~~ Implement ORDER BY BM25 Dec 6, 2024

jbellis added 2 commits December 6, 2024 16:30

implement BM25

237cec4

re-disallow DESC with ORDER BY ANN

56a6e0f

jbellis force-pushed the scoreordering-6 branch from 162e02e to 56a6e0f Compare December 6, 2024 22:30

cleanup and comments

30b6545

michaeljmarshall reviewed Dec 9, 2024

View reviewed changes

jbellis added 5 commits December 11, 2024 09:00

address review notes

6c9a0e6

remove unused limit parameter from IndexSearcher::search

e107fcc

eliminate currentRowIds

cfe204a

add testMatchingAllowed and make it work via shouldMerge

3d17e2f

jbellis force-pushed the scoreordering-6 branch from 5f5eb0e to 3d17e2f Compare December 11, 2024 21:36

jbellis added 2 commits December 12, 2024 08:31

disambiguate the BM25 error message when the index isn't analyzed

907a2ee

validateOptions treats analyzed and un-analyzed indexes as distinct, …

3315a12

…testTwoIndexes passes

jbellis force-pushed the scoreordering-6 branch from 01d3adc to b19fead Compare December 12, 2024 19:38

detect and reject ambiguous equality predicates; testAmbiguousPredica…

c0de416

…tes passes

jbellis force-pushed the scoreordering-6 branch 2 times, most recently from 2babc86 to 50c4a57 Compare December 13, 2024 14:36

don't inject +score unless coordinator requests it; this is a cleaner…

3967e7c

… approach than ignoring it when serialization fails later

don't inject ordering columns a second time into wildcard columns

db8d6b1

jbellis force-pushed the scoreordering-6 branch from 1cfbbba to a3d5c67 Compare February 21, 2025 22:08

jbellis added 6 commits February 21, 2025 16:20

add toString

9de486d

comments

c38b3e3

disable MonitorableImpl's abort when DISABLE_TIMEOUT is set

5f9bdab

MM fix to merge restoring SAI special case to isReversed

51a061d

remove vestigal parallel processing

a5c3089

split apart replica-side and coordinator-side processing since they m…

39652f1

…ostly do different things, jamming the logic together in "processPartitions" caused way more complexity than saving a repeated while loop was worth

jbellis force-pushed the scoreordering-6 branch from a3d5c67 to 39652f1 Compare February 21, 2025 23:31

jbellis added 4 commits February 24, 2025 12:51

fix vector binding and set euclidian similarity so data gets indexed

fc8d43c

AnnIndexScan needs to implement getIndexContext so metrics get increm…

bbdec82

…ented

Merge branch 'main' into scoreordering-6

c71e0df

whitespace change to poke CI

3f60526

michaeljmarshall reviewed Feb 25, 2025

View reviewed changes

michaeljmarshall approved these changes Feb 25, 2025

View reviewed changes

jbellis added 2 commits February 26, 2025 06:38

NON_DAEMON indexes claim to support everything and have null metadata…

8268866

… so code around that

Merge branch 'main' into scoreordering-6

4d7621c

jbellis merged commit b0cdc37 into main Feb 27, 2025
168 of 408 checks passed

jbellis deleted the scoreordering-6 branch February 27, 2025 00:44

michaeljmarshall restored the scoreordering-6 branch February 27, 2025 17:41

michaeljmarshall deleted the scoreordering-6 branch February 27, 2025 17:42

michaeljmarshall mentioned this pull request Feb 27, 2025

Fix EqBehavior bug introduced by #1434 #1614

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement ORDER BY BM25 #1434

Implement ORDER BY BM25 #1434

jbellis commented Nov 20, 2024 •

edited

Loading

jbellis commented Nov 22, 2024

michaeljmarshall left a comment

michaeljmarshall left a comment

jbellis commented Feb 14, 2025

michaeljmarshall commented Feb 19, 2025 •

edited

Loading

michaeljmarshall commented Feb 24, 2025

jbellis commented Feb 25, 2025

jbellis commented Feb 25, 2025

michaeljmarshall Feb 25, 2025

jbellis Feb 25, 2025

michaeljmarshall left a comment

sonarqubecloud bot commented Feb 27, 2025

cassci-bot commented Feb 27, 2025

Implement ORDER BY BM25 #1434

Implement ORDER BY BM25 #1434

Conversation

jbellis commented Nov 20, 2024 • edited Loading

What is the issue

What does this PR fix and why was it fixed

Checklist before you submit for review

jbellis commented Nov 22, 2024

michaeljmarshall left a comment

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

jbellis commented Feb 14, 2025

michaeljmarshall commented Feb 19, 2025 • edited Loading

michaeljmarshall commented Feb 24, 2025

jbellis commented Feb 25, 2025

jbellis commented Feb 25, 2025

michaeljmarshall Feb 25, 2025

Choose a reason for hiding this comment

jbellis Feb 25, 2025

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 27, 2025

Quality Gate passed

cassci-bot commented Feb 27, 2025

✔️ Build ds-cassandra-pr-gate/PR-1434 approved by Butler

jbellis commented Nov 20, 2024 •

edited

Loading

michaeljmarshall commented Feb 19, 2025 •

edited

Loading