Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

search + moves to drizzle + new landing fixes #329

Merged
merged 11 commits into from
Jan 19, 2025
Merged

search + moves to drizzle + new landing fixes #329

merged 11 commits into from
Jan 19, 2025

Conversation

dinmukhamedm
Copy link
Member

@dinmukhamedm dinmukhamedm commented Jan 19, 2025

Important

Enhance semantic search service with BM25 model support, sparse vector handling, and date range filtering in queries.

  • Semantic Search Service:
    • Added support for BM25 model in semantic_search/mod.rs and semantic_search_grpc.rs.
    • Updated Model enum to include BM25.
    • Implemented sparse vector handling for BM25 in vectordb/mod.rs.
    • Modified index and query methods to handle sparse indices.
  • gRPC Service:
    • Added DateRange and DateRanges to QueryRequest in semantic_search_grpc.rs.
    • Updated QueryPoint to use datapoint_id instead of content.
  • Vector Database:
    • Updated QdrantClient to support sparse vectors and date range filtering.
    • Added create_collection logic for sparse vectors in vectordb/mod.rs.

This description was created by Ellipsis for f579eb9. It will automatically update as commits are pushed.

nidhi-wa and others added 11 commits January 14, 2025 09:39
* Add ability to delete individual traces

* improve delete traces and span functionality

* update route handlers
* wip: store only metadata in qdrant

* further fixes

* add filter by metadata to api request
* wip: store spans in qdrant

* lint, fix build
* upload langchain b64 images to S3

* remove println
accomodate -> accommodate

---------
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to f579eb9 in 4 minutes and 1 seconds

More details
  • Looked at 10283 lines of code in 87 files
  • Skipped 6 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. app-server/src/api/v1/semantic_search.rs:100
  • Draft comment:
    Using unwrap here can lead to a panic if datapoint_id is not a valid UUID. Consider handling this error gracefully.
  • Reason this comment was not posted:
    Marked as duplicate.
2. app-server/src/api/v1/semantic_search.rs:102
  • Draft comment:
    Using Uuid::parse_str followed by unwrap can lead to panics if the string is not a valid UUID. Consider handling this more gracefully, perhaps by using expect with a clear error message or by returning a Result.
  • Reason this comment was not posted:
    Marked as duplicate.
3. frontend/components/ui/DeleteSelectedRows.tsx:15
  • Draft comment:
    Consider generalizing the existing DeleteDatapointsDialog component instead of creating a new one. The core functionality is identical.

  • component DeleteDatapointsDialog (delete-datapoints-dialog.tsx)

  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    While there is overlap in functionality, DeleteSelectedRows appears to be a more generic version already. It handles any entity type through the entityName prop, while DeleteDatapointsDialog is specific to datapoints with extra features. The new component seems to be the generalized version already, making the comment's suggestion backwards. The author likely intentionally created a generic version for broader reuse.
    I could be wrong about the intention - maybe there are other use cases where the datapoint-specific features would be valuable to preserve in a generalized component.
    The new component's simpler, more generic interface suggests a deliberate design choice to create a reusable component without datapoint-specific features.
    The comment should be deleted as it suggests generalizing in the wrong direction - the new component is already the generalized version.

Workflow ID: wflow_L65FNwCiQD4XPgTJ


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

.map(|result| SemanticSearchResult {
dataset_id,
.map(|result| SemanticSearchPoint {
datapoint_id: Uuid::parse_str(serde_json::from_str(&result.datapoint_id).unwrap())
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider handling potential errors from Uuid::parse_str to avoid panics if the string is not a valid UUID.

@dinmukhamedm dinmukhamedm merged commit 5d9ab0d into main Jan 19, 2025
2 checks passed
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.

4 participants