CNDB-13483: Fix loading PQ file when disabled_reads is true #1713

michaeljmarshall · 2025-04-25T22:40:03Z

What is the issue

Fixes: https://github.com/riptano/cndb/issues/13483

Initial Commits

CNDB-13483: Fix loading PQ file when disabled_reads is true
Refactor code to pull PQ fetching logic into single class
Add comment asking about reducing memory utilization when loading PQ
Fix the broken allRowsHaveVectorsInWrittenSegments method

What does this PR fix and why was it fixed

The cassandra.sai.disabled_reads prevents us from reading previous PQ objects during compaction, which leads to less efficient graph construction. This fixes that by adding a new searchable index that is capable of discovering/downloading/cleaning up the right index files to get that information.

I also have a tangential bug fix for code that was also impacted by the disabled_reads setting. However, there is a follow up question to ask if it is a valid implementation.

This fixes the method so that it actually reads the postings list structure. However, there is a larger question of whether the implementation was actually correct. It all comes down to what is in the indexContext's view on a compactor. We really only want to get the info from the source components that are being compacted. Further investigation needed to determine if this is the right behavior.

github-actions · 2025-04-25T22:40:21Z

michaeljmarshall · 2025-04-25T22:41:10Z

@jkni @eolivelli - I will add tests for this PR on Monday. The initial implementation is ready for review. Manual testing suggests that it works. I wanted to push it up to run the rest of CI to get that feedback by Monday

* cover building the vector index after creation * cover different methods within the V1MetadataOnlySearchableIndex

michaeljmarshall · 2025-05-07T22:25:23Z

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

+        // TODO should we load all of these for this op? It seems very unlikely that we want to consider the whole
+        // table when checking if all rows have vectors. A single empty vector will be enough to make this false,
+        // but that should really only impact that table.
+        var view = indexContext.getReferencedView(TimeUnit.SECONDS.toNanos(5));


I am moving this TODO out of scope for this PR. Here is a follow up ticket: https://github.com/riptano/cndb/issues/14028

eolivelli

LGTM

please put your tick on each line of the checklist: #1713 (comment)

eolivelli · 2025-05-09T14:05:33Z

src/java/org/apache/cassandra/index/sai/disk/V1MetadataOnlySearchableIndex.java

+    {
+        // May result in downloading file, but this metadata is valuable. We use a stream to avoid loading all the
+        // structures at once.
+        return metadatas.stream()


I appreciate the idea of using a Stream in order to not eagerly load stuff.
I think that Streams internally do some batching, if you want to not fall into that hidden behavior you could use a simple Iterator

Do you have a reference where I can learn more? The javadoc for the Stream interface indicates:

* Streams are lazy; computation on the source data is only performed when the * terminal operation is initiated, and source elements are consumed only * as needed.

I have not (I have only spent time in debugging problems in the past, but I don't have references)

we can keep this in the current form

src/java/org/apache/cassandra/index/sai/disk/V1MetadataOnlySearchableIndex.java

src/java/org/apache/cassandra/index/sai/disk/vector/ProductQuantizationFetcher.java

eolivelli · 2025-05-19T15:05:18Z

test/unit/org/apache/cassandra/index/sai/cql/VectorCompactionTest.java

+
+        // first sstable has one-to-one
+        for (int i = 0; i < MIN_PQ_ROWS; i++)
+            execute("INSERT INTO %s (pk, v) VALUES (?, ?)", i, randomVectorBoxed(2));


side comment (not asking for a change):
make randomVectorBoxed with only 2 dimensions could return duplicate vectors and make the test flaky

jkni

Approach looks good to me in general. I left some nits/questions inline. Is there a CNDB PR/have we exercised this code in CNDB yet?

src/java/org/apache/cassandra/index/sai/disk/format/OnDiskFormat.java

src/java/org/apache/cassandra/config/CassandraRelevantProperties.java

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

src/java/org/apache/cassandra/index/sai/disk/vector/ProductQuantizationFetcher.java

src/java/org/apache/cassandra/index/sai/disk/V1MetadataOnlySearchableIndex.java

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

sonarqubecloud · 2025-05-28T23:19:29Z

Quality Gate failed

Failed conditions
77.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

cassci-bot · 2025-05-28T23:25:02Z

❌ Build ds-cassandra-pr-gate/PR-1713 rejected by Butler

2 new test failure(s) in 7 builds
See build details here

Found 2 new test failures

Test	Explanation	Branch history	Upstream history
...ctorCompactionTest.testDelayedIndexCreation[ca]	regression	🔴🔵🔵🔵
o.a.c.u.b.BinLogTest.testTruncationReleasesLogS...	regression	🔴🔵🔵🔵🔴🔵🔵	🔵🔵🔵🔵🔵🔵🔵

Found 7 known test failures

cassci-bot · 2025-05-28T23:25:02Z

❌ Build ds-cassandra-pr-gate/PR-1713 rejected by Butler

1 new test failure(s) in 6 builds
See build details here

Found 1 new test failures

Test	Explanation	Branch history	Upstream history
...ctorCompactionTest.testDelayedIndexCreation[dc]	regression	🔴🔴🔵

Found 7 known test failures

eolivelli · 2025-06-05T08:59:49Z

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

+    @VisibleForTesting
+    public boolean areSegmentsLoaded()
+    {
+        return searchableIndex instanceof V1SearchableIndex;


instanceof smells here, cannot we push this method into SearchableIndex ?

eolivelli · 2025-06-05T09:01:08Z

src/java/org/apache/cassandra/index/sai/disk/V1MetadataOnlySearchableIndex.java

+    {
+        // May result in downloading file, but this metadata is valuable. We use a stream to avoid loading all the
+        // structures at once.
+        return metadatas.stream()


I have not (I have only spent time in debugging problems in the past, but I don't have references)

we can keep this in the current form

michaeljmarshall added 4 commits April 25, 2025 16:50

CNDB-13483: Fix loading PQ file when disabled_reads is true

504b382

Refactor code to pull PQ fetching logic into single class

60d19fe

Add comment asking about reducing memory utilization when loading PQ

7ffd115

michaeljmarshall requested review from jkni and eolivelli April 25, 2025 22:40

michaeljmarshall self-assigned this Apr 25, 2025

michaeljmarshall added 6 commits April 25, 2025 20:35

Log IOException and continue processing to prevent unnecessary flakiness

d3b2b22

Merge remote-tracking branch 'datastax/main' into cndb-13483

58fbc17

Cleanup unused throws in method signature

6e4b72d

Add test; minor fixes exposed by tests

bbfec37

Tweak postings strcuture logic (deeper changes to come)

0e3700b

Add more tests

cf9a2bd

* cover building the vector index after creation * cover different methods within the V1MetadataOnlySearchableIndex

michaeljmarshall commented May 7, 2025

View reviewed changes

Refactor API for getting postings structures

2c820b7

eolivelli approved these changes May 19, 2025

View reviewed changes

jkni reviewed May 19, 2025

View reviewed changes

michaeljmarshall added 5 commits May 28, 2025 15:11

Merge remote-tracking branch 'datastax/main' into cndb-13483

18f883e

Initial refactors after review

3748693

Push getting PqInfo into SearchableIndex interface

b5441cd

Fix comments; add version filtering to getPostingsStructures

a872344

More cleanup

e342824

eolivelli reviewed Jun 5, 2025

View reviewed changes

CNDB-13483: Fix loading PQ file when disabled_reads is true #1713

Are you sure you want to change the base?

CNDB-13483: Fix loading PQ file when disabled_reads is true #1713

Uh oh!

Conversation

michaeljmarshall commented Apr 25, 2025

What is the issue

Initial Commits

What does this PR fix and why was it fixed

Uh oh!

github-actions bot commented Apr 25, 2025

Checklist before you submit for review

Uh oh!

michaeljmarshall commented Apr 25, 2025

Uh oh!

michaeljmarshall May 7, 2025

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli May 9, 2025

Choose a reason for hiding this comment

Uh oh!

michaeljmarshall May 28, 2025

Choose a reason for hiding this comment

Uh oh!

eolivelli Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eolivelli May 19, 2025

Choose a reason for hiding this comment

Uh oh!

jkni left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented May 28, 2025

Quality Gate failed

Uh oh!

cassci-bot commented May 28, 2025

❌ Build ds-cassandra-pr-gate/PR-1713 rejected by Butler

Found 2 new test failures

Found 7 known test failures

Uh oh!

cassci-bot commented May 28, 2025

❌ Build ds-cassandra-pr-gate/PR-1713 rejected by Butler

Found 1 new test failures

Found 7 known test failures

Uh oh!

eolivelli Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

eolivelli Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkni left a comment •

edited

Loading