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

refac: Migrate all datasets db operation to drizzle on NextJs #197

Merged

Conversation

alviskalev
Copy link
Contributor

@alviskalev alviskalev commented Nov 12, 2024

closed #82


Important

Migrated dataset database operations from SQLx to Drizzle ORM in both backend and frontend, updating routes and pages accordingly.

  • Backend Migration:
    • Removed SQLx-based functions create_dataset, get_datasets, count_datasets, and rename_dataset from datasets.rs.
    • Removed corresponding routes from routes/datasets.rs.
    • Updated main.rs to remove unused dataset routes.
  • Frontend Migration:
    • Replaced fetcher calls with Drizzle ORM queries in route.ts for dataset operations.
    • Updated page.tsx to use Drizzle ORM for dataset retrieval.
  • Misc:
    • Removed unused imports and functions related to SQLx in affected files.

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

@CLAassistant
Copy link

CLAassistant commented Nov 12, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@dinmukhamedm dinmukhamedm left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's test and remove the relevant app-server code before we can merge this

@alviskalev
Copy link
Contributor Author

alviskalev commented Nov 12, 2024

hey @dinmukhamedm just wanna confirm before I delete this, this response any_in_project isn’t used in any frontend component, right?
I searched through all frontend components and didn’t see that field being used anywhere.
Screenshot 2024-11-12 at 22 18 47

@alviskalev alviskalev changed the title refac: Migrate get datasets operation to drizzle on NextJs refac: Migrate all datasets db operation to drizzle on NextJs Nov 12, 2024
@alviskalev
Copy link
Contributor Author

I think we no need to move the delete method for the dataset to Next.js, as there is semantic search that handles the deletion operation.
Screenshot 2024-11-12 at 22 40 54

@alviskalev
Copy link
Contributor Author

btw just info that the POST method for updating a dataset in frontend/app/api/projects/[projectId]/datasets/[datasetId]/route.ts is not used anywhere in the codebase

@dinmukhamedm
Copy link
Member

this response any_in_project isn’t used in any frontend component, right?

Yes, this is for consistency with get evaluations and get traces where we used it to show a page placeholder if there are none. However, this is already done separately in the frontend. We can remove this, this is safe.

I think we no need to move the delete method for the dataset to Next.js, as there is semantic search that handles the deletion operation.

Great catch! Let's keep it for now. Later we may want to move that request to the frontend too, but it's gRPC, so it is a little bit more work

btw just info that the POST method for updating a dataset in frontend/app/api/projects/[projectId]/datasets/[datasetId]/route.ts is not used anywhere in the codebase

yep, saw that, we can delete that.

@alviskalev alviskalev marked this pull request as ready for review November 13, 2024 05:31
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.

👍 Looks good to me! Reviewed everything up to fadd191 in 29 seconds

More details
  • Looked at 433 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. frontend/app/api/projects/[projectId]/datasets/[datasetId]/route.ts:22
  • Draft comment:
    The DELETE method is still using the fetcher utility to make a DELETE request to the backend. Consider refactoring this to use Drizzle ORM directly for database operations, similar to the GET method.
  • Reason this comment was not posted:
    Comment was on unchanged code.
2. frontend/app/api/projects/[projectId]/datasets/route.ts:56
  • Draft comment:
    The DELETE method is still using the fetcher utility to make a DELETE request to the backend. Consider refactoring this to use Drizzle ORM directly for database operations, similar to the GET method.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_htFESdgvi96DeWwt


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Member

@dinmukhamedm dinmukhamedm left a comment

Choose a reason for hiding this comment

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

Thanks a ton!

@dinmukhamedm dinmukhamedm merged commit b99779b into lmnr-ai:dev Nov 13, 2024
1 check 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.

3 participants