Skip to content

feat: add search for vector, keyword, and hybrid search - #82

Closed
lukekim wants to merge 4 commits into
trunkfrom
feat/search
Closed

feat: add search for vector, keyword, and hybrid search#82
lukekim wants to merge 4 commits into
trunkfrom
feat/search

Conversation

@lukekim

@lukekim lukekim commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

What

Adds SpiceClient::search, wrapping the runtime's POST /v1/search — vector similarity, keyword, and hybrid search over datasets with an embedding column.

let response = client
    .search(
        SearchRequest::new("tickets to Tokyo")
            .with_datasets(["app_messages"])
            .with_limit(3),
    )
    .await?;

for m in &response {
    println!("{} {} {:?}", m.dataset, m.score, m.matches);
}

SearchRequest follows the crate's existing builder convention (mut selfSelf), so only the query text is required. with_keywords pre-filters the embedding column with a lexical search before the vector search runs, making the search hybrid.

New public exports: SearchRequest, SearchResponse, SearchMatch, SearchError.

Why

/v1/search works on a default spice run and is a distinctive Spice capability, but Rust users had no way to reach it short of hand-rolling the HTTP call. spice.js is currently the only SDK that exposes it.

Two wire-format details the types handle so callers don't have to:

  • the similarity score is serialized as _score, not score, and the request's SQL predicate is where, not where_cond
  • matches, data, primary_key and metadata are omitted entirely when empty, so they deserialize to empty maps — readable without a guard

Errors go through a SearchError enum consistent with DatasetError and QueryError, and surface the runtime's own message on a rejected search rather than a raw transport error.

Part of aligning search support across the SDKs.

Verification

  • cargo build
  • cargo test --lib — 197 passed, including 6 serde tests in search.rs and 4 wiremock-backed client tests covering request encoding, response decoding, empty-text rejection, and runtime error surfacing
  • cargo test --doc — 29 passed, including the new README examples (the crate doctests its README via include_str!)
  • cargo fmt --all --check
  • cargo clippy --all-features — no new warnings
  • Verified against a live spice run: the search request reaches /v1/search, and a
    failure surfaces the runtime's own message ("Search cannot be run on <dataset> because it has no embeddings or full text search indexes.") rather than a bare status code
  • A search returning matches was not exercised end to end — that needs a dataset with an
    embedding column and a loaded embedding model, which this environment has no credentials for. The 24 tests/client_test.rs failures in a full cargo test are all refused Flight connections and are unrelated to this change.

Wraps POST /v1/search, which was previously unreachable from Rust without
hand-rolling the HTTP call. spice.js is the only other SDK that exposes it.

SearchRequest follows the crate's builder convention — only the query text is
required, and with_keywords pre-filters the embedding column with a lexical
search before the vector search, making the search hybrid.

SearchMatch deserializes the runtime's wire format, including the `_score`
field name and the objects the runtime omits when empty; those decode to empty
maps so callers can read them without a guard.
Copilot AI review requested due to automatic review settings July 27, 2026 15:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds first-class Rust SDK support for the Spice runtime’s POST /v1/search endpoint by introducing typed request/response models and a new SpiceClient::search method, enabling vector, keyword, and hybrid search over embedding-enabled datasets.

Changes:

  • Introduces SearchRequest, SearchResponse, SearchMatch, and SearchError with serde wire-format handling (e.g., where, _score, omitted empty maps).
  • Adds SpiceClient::search and the underlying HTTP call in QueryHttpClient.
  • Exports the new search types publicly and documents usage in the README (plus adds wiremock/serde tests).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/search.rs New public search request/response/error types, builder API, and serde tests for wire-format compatibility.
src/query.rs Adds HTTP client method to POST /v1/search and decode the response (or surface runtime errors).
src/lib.rs Wires in the new module and re-exports the public search API types.
src/client.rs Adds SpiceClient::search wrapper with validation, docs, and wiremock-backed client tests.
README.md Documents the new search() capability with runnable examples (doctested).
Comments suppressed due to low confidence (2)

src/search.rs:109

  • with_additional_columns serializes an empty iterator as "additional_columns": [] because it sets Some(empty_vec). Since empty lists typically mean “no extra columns” (i.e., omit the field), treating empty as None avoids sending a value that may have different semantics than omission.
    pub fn with_additional_columns<I, S>(mut self, columns: I) -> Self
    where
        I: IntoIterator<Item = S>,
        S: Into<String>,
    {
        self.additional_columns = Some(columns.into_iter().map(Into::into).collect());
        self
    }

src/search.rs:119

  • with_keywords currently serializes an empty iterator as "keywords": [] (because it sets Some(empty_vec)). If empty keywords should behave the same as “no keywords”, it’s clearer to omit the field by treating empty as None.
    pub fn with_keywords<I, S>(mut self, keywords: I) -> Self
    where
        I: IntoIterator<Item = S>,
        S: Into<String>,
    {
        self.keywords = Some(keywords.into_iter().map(Into::into).collect());
        self
    }

Comment thread src/search.rs
@lukekim lukekim self-assigned this Jul 27, 2026
@lukekim
lukekim requested a review from sgrebnov July 27, 2026 18:08
@lukekim lukekim added the enhancement New feature or request label Jul 27, 2026
@lukekim lukekim added this to the v4.0.0 milestone Jul 27, 2026
lukekim added 2 commits July 27, 2026 15:41
…gistry

`spice add spiceai/quickstart` makes every run depend on the Spicepod registry,
which is currently returning a body the CLI cannot unpack:

  Invalid argument: Failed to extract Spicepod archive: invalid Zip archive:
  Could not find EOCD

Declaring the same dataset inline removes the dependency. Covers both the Unix
step (heredoc) and the Windows step (PowerShell here-string).

Verified locally: taxi_trips loads and is queryable within seconds.

Mirrors spicepy#176.
Copilot AI review requested due to automatic review settings July 29, 2026 21:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

src/search.rs:44

  • SearchRequest derives Default, but an empty text is invalid (and SpiceClient::search explicitly rejects it). Exposing SearchRequest::default() makes it easy for callers to construct an invalid request and contradicts the docs that imply SearchRequest::new(text) is the required entrypoint. Consider removing the Default derive (and constructing fields explicitly in new) to avoid exposing an invalid default instance.
#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize)]
pub struct SearchRequest {
    /// The query to find similar documents for.
    pub text: String,

src/query.rs:716

  • unwrap_or_default() on response.text().await silently drops body-read failures, which can make diagnosing non-200 responses harder (you'll report an empty response_body). Consider preserving the error context in the response_body string when the body can't be read.
                let response_body = response.text().await.unwrap_or_default();

Review asked whether with_datasets(empty) should coerce to None. It should not:
an empty list asks for a search over no datasets and the runtime says so, while
None widens the search to every dataset with an embedding column. A caller whose
list came out empty is better served by the error than by a silent fan-out, and
gospice, spicepy and spice-dotnet all send an explicit empty list too.

Documents the distinction on the field and pins it with a test.
Copilot AI review requested due to automatic review settings July 29, 2026 23:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/client.rs:600

  • SpiceClient::search only checks request.text.is_empty(), so a whitespace-only query (e.g. " ") is treated as valid and will be sent to the runtime. Given the API docs/error variant (EmptyText) imply “no query text”, trimming before validation avoids an unnecessary network round trip and keeps client-side validation consistent with intent.
        if request.text.is_empty() {
            return Err(SearchError::EmptyText);
        }

@claudespice

Copy link
Copy Markdown
Contributor

This PR is CONFLICTING / DIRTY against trunk, and since there is no notification for that state I wanted to flag it along with the cause — it is narrower than the 9-day age suggests.

One commit is responsible. Exactly one thing has landed on trunk since this branch was last pushed (2026-07-29): 3267ff355"fix: keep the API key on the origin it was configured for" (#85), merged 2026-08-05T03:15Z. So the conflict is roughly 12 hours old, not 9 days.

It is mechanical — the two sides do not disagree about anything. They touch the same three files in different places:

file #85 this PR
src/lib.rs adds mod redirect; + re-export adds mod search; + re-export
src/client.rs SpiceClientBuilder::build (~L755), test helper (~L817) use block (~L9), SpiceClient::search (~L560)
src/query.rs QueryHttpClient::new (~L454), test helpers (~L1394+) QueryHttpClient::search (~L691)

The genuine textual collision is in lib.rs, where both changes insert into the same module list and the same pub use block a line apart. Both additions are kept; there is nothing to choose between.

The one thing worth checking, which also turns out to be clear: #85 made QueryHttpClient::new fallible (-> Result<Self, reqwest::Error>) and gated it behind #[cfg(test)]. That would be a real problem for any new production construction path — but this branch adds none. Its search methods go through self.client / add_auth on an already-built client, so the new signature does not reach any code this PR adds.

Not resolving it myself, deliberately. I have a competing PR — #84 adds the same feature from a fork, and I flagged there earlier today that this PR implements it and was opened first. Which of the two lands is your call, not something I should pre-empt by pushing to this branch. Happy to do the merge if you'd rather I did; it should be a short one.

Worth noting for sequencing: this PR and #83 are the two that unblock the SDK search work more broadly — the fork-based PRs cannot go green until the CI-side fixes land.

@sgrebnov

Copy link
Copy Markdown
Contributor

Closing as superseded — search landed via #84 and the CI fix via #83.

Trunk already covers everything here:

  • SpiceClient::search(SearchRequest) -> Result<SearchResponse, SearchError> — identical signature
  • Full SearchRequest builder (with_datasets, with_limit, with_where, with_additional_columns, with_keywords for hybrid search)
  • Same wire-format handling (_score/where renames, defaulted empty maps)
  • The build.yml inline-dataset fix — landed in ci: define the test dataset inline instead of fetching it from the registry #83
  • Equivalent test coverage (4 wiremock client tests + 8 unit tests on trunk), with slightly stronger validation (also rejects limit == 0)

No remaining functionality to merge from this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants