CNDB-15619: Allow to customize SAI format to write and to consider as current() depending on a keyspace #2041

eolivelli · 2025-10-08T21:09:13Z

What is the issue

See https://github.com/riptano/cndb/issues/15619

What does this PR fix and why was it fixed

Introduce an callback to get the SAI version to use for a specific keyspace (defaults to cassandra.sai.latest.versison)
Introduce a Optional parameter to Version.current() and call it everywhere apart from tests
The new code should be "faster" because in many places we used to call current() that in turn reads a volatile variable, now on some hot paths we are using a final field (in IndexContext or other places)

CNDB side implementation: https://github.com/riptano/cndb/issues/15619

github-actions · 2025-10-08T21:09:28Z

adelapena · 2025-10-10T11:58:17Z

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

        /** Cost to begin processing PKs into index ordinals for estimateAnnSortCost */
        // DC introduced the one-to-many ordinal mapping optimization
-        public final static double ANN_SORT_OPEN_COST = Version.current().onOrAfter(Version.DC) ? 370 : 4200;
+        public final static double ANN_SORT_OPEN_COST = Version.current().onOrAfter(Version.DC) ? 370 : 4200; // it's okay to make this depending on the main SAI version


@eolivelli why is this ok?

This is okay because in production we use 'EC' or 'AA' everywhere

where we use 'AA' there is no vector search
so ALL the production clusters that support vector search use 'EC' or later
we can remove this check and make it a fixed constant

We can’t let keyspaces on AA use ORDER BY.
is this just Ann order by or also the generic one?
If this is just for ANN then it’s ok. The base will be EC and we won’t let people make a vector index to attempt ANN if their keyspace doesn’t support EC.

Having a quick look, it seems that this and the following ANN_SORT_KEY_COST definitions are the only places (outside of tests) that use Version.current() (the version without a keyspace).

I strongly suggest we fix this, remove Version.current() and have the version selection take String instead of Optional<String>. IMO, the concept of a "main" SAI version is confusing and is dangerous (makes it way too easy to rely on the default with debatable justification), so we should eliminate if it's not strictly needed (and it doesn't seem it is).

Thanks for the feedback. I have removed Version.current() and Version.CURRENT, so Version.current(String) is the only door. VersionSelector is moved to Version.Selector, which doesn't have an optional keyspace argument anymore.

adelapena · 2025-10-10T12:36:59Z

Setting as ready to review so CI runs on push, and added [WIP] (work in progress) to the title.

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

eolivelli · 2025-10-14T15:10:11Z

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

            Preconditions.checkArgument(!sealed, "Should not add components for SSTable %s at this point; the completion marker has already been written", descriptor);
            // When a sstable doesn't have any complete group, we use a marker empty one with a generation of -1:
-            Preconditions.checkArgument(buildId != EMPTY_GROUP_MARKER, "Should not be adding component to empty components");
+            // TODO....


I didn't find a good way to implement this assertion, maybe we can remove the commented code ?

I have added back the checks using a memoized instance of the empty group marker for the version.

pcmanus

Couple of very minor nits, but lgtm otherwise.

pcmanus · 2025-10-15T06:31:30Z

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

        IndexComponentsImpl components;
        if (buildId == null)
        {
+            Version version = context == null ? Version.current(descriptor.ksname) : context.version();


That line is dead code/unused.

pcmanus · 2025-10-15T06:37:31Z

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

            });
        }

+        @Override


What is the goal of adding that override? The default implementation in IndexComponents already returns buildId().version(), so it seems this override returns the same thing is isn't necessary. Obviously not a big deal even if unnecessary but ...

pcmanus · 2025-10-15T06:46:51Z

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

             var indexWriter = new OnDiskGraphIndexWriter.Builder(builder.getGraph(), indexFile.toPath())
                               .withStartOffset(termsOffset)
-                               .withVersion(Version.current().onDiskFormat().jvectorFileFormatVersion())
+                               .withVersion(perIndexComponents.context().version().onDiskFormat().jvectorFileFormatVersion())


Nit: the .context() part can be removed (at least for consistency with other calls in this file :)).

Right, removed.

pcmanus · 2025-10-15T06:49:33Z

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

-            CassandraOnHeapGraph.writePqHeader(pqOutput.asSequentialWriter(), unitVectors, VectorCompression.CompressionType.PRODUCT_QUANTIZATION);
-            compressedVectors.write(pqOutput.asSequentialWriter(), Version.current().onDiskFormat().jvectorFileFormatVersion());
+            CassandraOnHeapGraph.writePqHeader(pqOutput.asSequentialWriter(), unitVectors, VectorCompression.CompressionType.PRODUCT_QUANTIZATION, perIndexComponents.version());
+            compressedVectors.write(pqOutput.asSequentialWriter(), context.version().onDiskFormat().jvectorFileFormatVersion());


Nit: we use perIndexComponents.version() the line above, and context.version() here. It's obviously not a big deal since the value is always going to be the same, but that also feel a tad confusing for people not as familiar with the code, that on seeing it done differently may expect those 2 things aren't the same. Tl;dr, I think it's worth being consistent to facilitate future readers.

Right, I have put the context's version in a local var and used it on all the calls.

adelapena · 2025-10-15T10:33:02Z

Thanks for the review. I think I've addressed all the suggestions. Now I'll rebase and squash, and start working on a port to cndb-main-release-202505.

… current() depending on a keyspace There is a new cassandra.sai.version.selector.class system property allowing to provide an implementation of the o.a.c.index.sai.disk.format.Version.Selector interface to specify that version of the SAI on-disk index format should be used for each keyspace.

sonarqubecloud · 2025-10-15T11:34:53Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
91.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

cassci-bot · 2025-10-15T11:37:34Z

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

1 regressions found
See build details here

Found 1 new test failures

Test	Explanation	Runs	Upstream
o.a.c.index.sai.cql.VectorKeyRestrictedOnPartitionTest.partitionRestrictedWidePartitionBqCompressedTest[eb] (compression)	REGRESSION	🔵🔴	0 / 10

No known test failures found

adelapena · 2025-10-16T10:21:15Z

The occasional failure of VectorKeyRestrictedOnPartitionTest.partitionRestrictedWidePartitionBqCompressedTest seems due to a recall slightly worse than the expected 10%, similar to https://github.com/riptano/cndb/issues/12778, so likely not caused by the changes. I haven't been able to reproduce it locally.

… current() depending on a keyspace (#2064) There is a new cassandra.sai.version.selector.class system property allowing to provide an implementation of the o.a.c.index.sai.disk.format.Version.Selector interface to specify that version of the SAI on-disk index format should be used for each keyspace. This is a cherry-pick of #2041 Co-authored-by: Enrico Olivelli <[email protected]>

adelapena reviewed Oct 10, 2025

View reviewed changes

adelapena changed the title ~~CNDB-15619: Allow to customize SAI format to write and to consider as current() depending on a keyspace~~ [WIP] CNDB-15619: Allow to customize SAI format to write and to consider as current() depending on a keyspace Oct 10, 2025

adelapena marked this pull request as ready for review October 10, 2025 12:36

eolivelli commented Oct 14, 2025

View reviewed changes

src/java/org/apache/cassandra/index/sai/disk/format/IndexComponents.java Outdated Show resolved Hide resolved

eolivelli commented Oct 14, 2025

View reviewed changes

adelapena changed the title ~~[WIP] CNDB-15619: Allow to customize SAI format to write and to consider as current() depending on a keyspace~~ CNDB-15619: Allow to customize SAI format to write and to consider as current() depending on a keyspace Oct 14, 2025

pcmanus approved these changes Oct 15, 2025

View reviewed changes

adelapena force-pushed the cndb-15619-sai-per-tenant branch from be59b99 to 8eba026 Compare October 15, 2025 10:52

adelapena mentioned this pull request Oct 15, 2025

CNDB-15619: Allow to customize SAI format to write and to consider as current() depending on a keyspace #2064

Merged

adelapena merged commit 82064a0 into main Oct 16, 2025
497 of 498 checks passed

adelapena deleted the cndb-15619-sai-per-tenant branch October 16, 2025 16:50

CNDB-15619: Allow to customize SAI format to write and to consider as current() depending on a keyspace #2041

CNDB-15619: Allow to customize SAI format to write and to consider as current() depending on a keyspace #2041

Uh oh!

Conversation

eolivelli commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the issue

What does this PR fix and why was it fixed

Uh oh!

github-actions bot commented Oct 8, 2025 • edited by adelapena Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist before you submit for review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli Oct 13, 2025 • 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!

JeremiahDJordan Oct 13, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adelapena commented Oct 10, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcmanus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adelapena commented Oct 15, 2025

Uh oh!

sonarqubecloud bot commented Oct 15, 2025

Quality Gate passed

Uh oh!

cassci-bot commented Oct 15, 2025

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

Found 1 new test failures

No known test failures found

Uh oh!

adelapena commented Oct 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eolivelli commented Oct 8, 2025 •

edited

Loading

github-actions bot commented Oct 8, 2025 •

edited by adelapena

Loading

eolivelli Oct 13, 2025 •

edited

Loading

JeremiahDJordan Oct 13, 2025 •

edited

Loading