Skip to content

Conversation

@maedhroz
Copy link
Contributor

No description provided.

@maedhroz
Copy link
Contributor Author

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clohfink Without setting the task here, it's possible to miss tasks, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the simulator changes, this bit wouldn't have been necessary, as everything got wrapped up in FutureTask. Is there a better way?

CC @belliottsmith

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's similar to what we still do w/ FutureTask for NTR stage requests (and did for everything in the old patch).

@maedhroz maedhroz force-pushed the CASSANDRA-15241-trunk-v2 branch from a095235 to 6f78148 Compare June 28, 2022 03:57
@maedhroz maedhroz requested a review from belliottsmith June 28, 2022 19:53
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about the micros resolution. Would nanos suffice? Maybe there's a good motivation for micros

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This I copied from @clohfink 's original patch. I think most of the other metrics we expose around latencies/running times are in micros.

@maedhroz maedhroz force-pushed the CASSANDRA-15241-trunk-v2 branch from 4b57272 to 5abed93 Compare July 8, 2022 17:30
maedhroz and others added 2 commits July 8, 2022 14:53
patch by Chris Lohfink; reviewed by Caleb Rackliffe and Benedict Elliott Smith for CASSANDRA-15241

Co-authored-by: Chris Lohfink <clohfink@apple.com>
Co-authored-by: Caleb Rackliffe <calebrackliffe@gmail.com>
Co-authored-by: Benedict Elliott Smith <benedict@apache.org>
@maedhroz maedhroz force-pushed the CASSANDRA-15241-trunk-v2 branch from 5abed93 to 6dfdc93 Compare July 8, 2022 19:53
@maedhroz
Copy link
Contributor Author

maedhroz commented Jul 8, 2022

Committed as 89f3978

@maedhroz maedhroz closed this Jul 8, 2022
ekaterinadimitrova2 pushed a commit that referenced this pull request Jun 9, 2025
### What is the issue
Fixes riptano/cndb#13822

CNDB test pr riptano/cndb#13826

### What does this PR fix and why was it fixed

SAI’s DiskANN/JVector engine currently **always searches with pruning
enabled**, trading recall for latency. That is fine for most workloads,
but:

* **Threshold / bounded ANN queries** can lose matches when pruning
exits early.
* Performance‑testing users need an easy way to turn pruning off to
measure the recall/latency curve.

This patch introduces a per‑query **`use_pruning`** option so users and
operators can choose the trade‑off that suits them.

---

#### New query option

```cql
WITH ann_options = {'use_pruning': true|false}
```

*When omitted we fall back to the node level default (see below).*  

#### Default behaviour

* Cluster‑wide default is controlled by the JVM system property:
  ```
  -Dcassandra.sai.jvector.use_pruning_default=<true|false>
  ```
  exposed as `V3OnDiskFormat.JVECTOR_USE_PRUNING_DEFAULT`.
* The property defaults to `true`, preserving existing behaviour.

#### Validation rules

* Value must be the literal `true` or `false` (case‑insensitive).
* Unknown ANN option keys continue to raise `InvalidRequestException`.

#### Usage

* `Orderer` computes the value of the `usePruning` option by using the
`use_pruning` value if it is not null or the jvm default if it is null
and passes it down to **all** `graph.search()` calls.
* Threshold / bounded ANN queries always pass `use_pruning = false`
because correctness > latency for those paths (this is a net new change,
but it's very minor and might not have any impact on those queries
depending on the jvector implementation)

#### Compatibility

* We added one flag bit to `ANNOptions` serialization; older nodes
ignore unknown bits, so mixed‑version clusters are fine (though they do
throw an exception for unknown settings)

#### Tests added / updated

* Parsing, validation and transport round‑trips (`ANNOptionsTest`).
* Distributed smoke (`ANNOptionsDistributedTest`).
* Recall regression for pruning vs no‑pruning
(`VectorSiftSmallTest.ensureDisablingPruningIncreasesRecall`).
michaeljmarshall added a commit to michaeljmarshall/cassandra that referenced this pull request Sep 17, 2025
michaelsembwever pushed a commit to thelastpickle/cassandra that referenced this pull request Jan 7, 2026
Fixes riptano/cndb#13822

CNDB test pr riptano/cndb#13826

SAI’s DiskANN/JVector engine currently **always searches with pruning
enabled**, trading recall for latency. That is fine for most workloads,
but:

* **Threshold / bounded ANN queries** can lose matches when pruning
exits early.
* Performance‑testing users need an easy way to turn pruning off to
measure the recall/latency curve.

This patch introduces a per‑query **`use_pruning`** option so users and
operators can choose the trade‑off that suits them.

---

```cql
WITH ann_options = {'use_pruning': true|false}
```

*When omitted we fall back to the node level default (see below).*

* Cluster‑wide default is controlled by the JVM system property:
  ```
  -Dcassandra.sai.jvector.use_pruning_default=<true|false>
  ```
  exposed as `V3OnDiskFormat.JVECTOR_USE_PRUNING_DEFAULT`.
* The property defaults to `true`, preserving existing behaviour.

* Value must be the literal `true` or `false` (case‑insensitive).
* Unknown ANN option keys continue to raise `InvalidRequestException`.

* `Orderer` computes the value of the `usePruning` option by using the
`use_pruning` value if it is not null or the jvm default if it is null
and passes it down to **all** `graph.search()` calls.
* Threshold / bounded ANN queries always pass `use_pruning = false`
because correctness > latency for those paths (this is a net new change,
but it's very minor and might not have any impact on those queries
depending on the jvector implementation)

* We added one flag bit to `ANNOptions` serialization; older nodes
ignore unknown bits, so mixed‑version clusters are fine (though they do
throw an exception for unknown settings)

* Parsing, validation and transport round‑trips (`ANNOptionsTest`).
* Distributed smoke (`ANNOptionsDistributedTest`).
* Recall regression for pruning vs no‑pruning
(`VectorSiftSmallTest.ensureDisablingPruningIncreasesRecall`).
michaelsembwever pushed a commit to thelastpickle/cassandra that referenced this pull request Jan 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants