CNDB-16203: Make the counters in QueryContext plain longs #2153

adelapena · 2025-12-04T18:00:05Z

What is the issue

We don't need all the counters in QueryContext to be LongAdder, since they are only accessed by the query's thread.

They used to be plain longs until VECTOR-SEARCH-29 parallelized vector searches, making them accessed by multiple threads. However, that parallelization was removed by the BM25 patch

The system properties USE_PARALLEL_INDEX_READ and PARALLEL_INDEX_READ_NUM_THREADS are unused since BM25 removed parallel search.

What does this PR fix and why was it fixed

This PR makes the counters in QueryContext plain longs, without any concurrency control within the class.

The system properties USE_PARALLEL_INDEX_READ and PARALLEL_INDEX_READ_NUM_THREADS are removed, since they were no-op since the introduction of BM25. Since we don't reject unknown system properties, they will remain no-op.

github-actions · 2025-12-04T18:00:22Z

pkolaczk

The QueryContext changes looks good, but I suggest removing the not-used options instead of deprecating and optionally adding thread safety checks.

pkolaczk · 2025-12-08T11:46:47Z

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


    // Enables parallel index read.
+    @Deprecated // unused since BM25 removed parallel index read
    USE_PARALLEL_INDEX_READ("cassandra.index_read.parallel", "true"),


I think Deprecated should be used to mark things that technically still work but are going to be removed soon so to give the callers/users some time to stop using them. But it looks like we don't use this option anymore at all, and additionally, it doesn't work (flipping it doesn't change anything). I suggest removing it totally, because otherwise it only adds confusion.

I have a bit of a different experience.
We add the @Deprecated so anyone who thinks they are using it (it doesn't work anymore) can take the time to remove it and not just get broken straight.

This is no difference than a cassandra.yaml property (be careful with backward compatibility, breaking changes, etc). I'd say deprecate here, remove in main-5.0, to be on the safe side. WDYT? We should also document this.

@ekaterinadimitrova2 that was my initial reasoning leading to deprecate rather than remove the properties.

However, it seems that, differently to yaml properties, unknown system properties are not rejected. So startup won't fail if we remove the properties, it will simply be no-op. We don't have any entrypoints to dynamically change these property either.

If we leave them as deprecated, it doesn't seem that there is anything to warn about the usage of a deprecated system property. So starting with our without the property will be no-op too.

Normally I would agree that removing or even silently making a property no-op is not ideal. But in our case this was already no-op before, since the introduction of BM25. I think we are not really changing anything here, just finishing the cleanup of dead code left by BM25.

However, it seems that, differently to yaml properties, unknown system properties are not rejected.

Yes, they are not. But if we warn - then people will know to remove them/clean/etc. Otherwise it will be weird dead code (anyone using -D in their scripts, etc) for anyone who was using them with wrong expectations? (it won't break as they are not used internally)

But in our case this was already no-op before, since the introduction of BM25.

That is a good point that most people should have realized it up to now... Let's just put a note then and clean it. No better cleaning than removing "stuff" :)

I have added a note in the the issue's release notes. It says that they were no-op, and now they are still no-op too, which makes me think we might don't need the note at all.

which makes me think we might don't need the note at all.

No-op on our end, but I think the note is ok - it signals people they can do some cleaning on their end if they were using the -D flag and reduce confusion and amount of flags used in their clusters.

pkolaczk · 2025-12-08T11:46:56Z

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

+    @Deprecated // unused since BM25 removed parallel index read
    USE_PARALLEL_INDEX_READ("cassandra.index_read.parallel", "true"),
+    @Deprecated // unused since BM25 removed parallel index read
    PARALLEL_INDEX_READ_NUM_THREADS("cassandra.index_read.parallel_thread_num"),


Same as above

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

ekaterinadimitrova2

The "new" test failures are timeouts we see in CI which shouldn't be related to what we do here. Maybe run the tests locally one more time before commit to ensure we are not missing anything as they are in the SAI area - just in case.

ekaterinadimitrova2 · 2025-12-09T22:07:37Z

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


-    // Enables parallel index read.
-    USE_PARALLEL_INDEX_READ("cassandra.index_read.parallel", "true"),
-    PARALLEL_INDEX_READ_NUM_THREADS("cassandra.index_read.parallel_thread_num"),


nit: You may want to update the PR description to say they are removed

Yes, I have updated the description of both PRs.

ekaterinadimitrova2 · 2025-12-09T22:09:33Z

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


    // Enables parallel index read.
+    @Deprecated // unused since BM25 removed parallel index read
    USE_PARALLEL_INDEX_READ("cassandra.index_read.parallel", "true"),


which makes me think we might don't need the note at all.

No-op on our end, but I think the note is ok - it signals people they can do some cleaning on their end if they were using the -D flag and reduce confusion and amount of flags used in their clusters.

We don't need all the counters in QueryContext to be LongAdder, since they are only accessed by the query's thread. They used to be plain longs until VECTOR-SEARCH-29 parallelized vector searches, making them accessed by multiple threads. However, that parallelization was removed by the BM25 patch. The system properties USE_PARALLEL_INDEX_READ and PARALLEL_INDEX_READ_NUM_THREADS are marked as deprecated, since they are unused since the introduction of BM25.

Metrics are generally read from the context snapshot

sonarqubecloud · 2025-12-10T17:34:05Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarQube Cloud

cassci-bot · 2025-12-10T17:41:39Z

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

2 regressions found
See build details here

Found 2 new test failures

Test	Explanation	Runs	Upstream
o.a.c.index.sai.cql.VectorCompaction100dTest.testCompactionWithEnoughRowsForPQAndDeleteARow[eb false]	NEW	🔴⚪	0 / 19
o.a.c.index.sai.cql.VectorSiftSmallTest.testCompaction[db false]	REGRESSION	🔴⚪	0 / 19

Found 2 known test failures

adelapena self-assigned this Dec 4, 2025

pkolaczk requested changes Dec 8, 2025

View reviewed changes

adelapena force-pushed the CNDB-16203-main-query-context-longs branch from 0daedcc to bc700c4 Compare December 8, 2025 13:48

adelapena requested review from ekaterinadimitrova2 and pkolaczk December 9, 2025 16:30

ekaterinadimitrova2 approved these changes Dec 9, 2025

View reviewed changes

adelapena added 3 commits December 10, 2025 10:33

CNDB-16203: Remove unnecessary getters from QueryContext

ad8b7c3

Metrics are generally read from the context snapshot

CNDB-16203: Address review feedback

adfda82

adelapena force-pushed the CNDB-16203-main-query-context-longs branch from bc700c4 to adfda82 Compare December 10, 2025 11:54

pkolaczk approved these changes Dec 10, 2025

View reviewed changes

Empty-Commit to trigger CI

98e5a62

adelapena merged commit c093487 into main Dec 10, 2025
489 of 497 checks passed

adelapena deleted the CNDB-16203-main-query-context-longs branch December 10, 2025 17:51

CNDB-16203: Make the counters in QueryContext plain longs #2153

CNDB-16203: Make the counters in QueryContext plain longs #2153

Uh oh!

Conversation

adelapena commented Dec 4, 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 Dec 4, 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!

pkolaczk 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!

ekaterinadimitrova2 Dec 8, 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!

ekaterinadimitrova2 Dec 8, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ekaterinadimitrova2 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!

sonarqubecloud bot commented Dec 10, 2025

Quality Gate passed

Uh oh!

cassci-bot commented Dec 10, 2025

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

Found 2 new test failures

Found 2 known test failures

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adelapena commented Dec 4, 2025 •

edited

Loading

github-actions bot commented Dec 4, 2025 •

edited by adelapena

Loading

ekaterinadimitrova2 Dec 8, 2025 •

edited

Loading

ekaterinadimitrova2 Dec 8, 2025 •

edited

Loading