Skip to content

Conversation

@averikitsch
Copy link
Collaborator

No description provided.

@eyurtsev eyurtsev self-requested a review April 9, 2025 16:18
@eyurtsev
Copy link
Collaborator

eyurtsev commented Apr 9, 2025

I would suggest implementing on PGEngine for now rather than the vectorstore interface. To mirror the creation API.

I'd suggest re-organizing the API longer-term to decouple schema management from using the vectorstore; i.e., I don't want to have to initialize a vectorstore w/ an embedding service just to delete a table.

  • PGEngine should be the engine / connection pool etc
  • Vectorstore for managing an existing table (add docs, delete, search)

VectorstoreManager (or some other name) accepts PGEngine and is responsible for managing schema:

  • create tables
  • delete tables
  • list tables w/ introspection
  • get a vectorstore by ID
  • index application should live here as well probably?

@averikitsch
Copy link
Collaborator Author

I would suggest implementing on PGEngine for now rather than the vectorstore interface. To mirror the creation API.

I'd suggest re-organizing the API longer-term to decouple schema management from using the vectorstore; i.e., I don't want to have to initialize a vectorstore w/ an embedding service just to delete a table.

  • PGEngine should be the engine / connection pool etc
  • Vectorstore for managing an existing table (add docs, delete, search)

VectorstoreManager (or some other name) accepts PGEngine and is responsible for managing schema:

  • create tables
  • delete tables
  • list tables w/ introspection
  • get a vectorstore by ID
  • index application should live here as well probably?

Agreed! Thanks for the notes. Let's chat more about the VSManager

@eyurtsev
Copy link
Collaborator

@averikitsch feel free to merge. I'd prefer this on PGEngine, but also if we introduce another intermediate entity later we'll deprecate it from either location.

@averikitsch
Copy link
Collaborator Author

@averikitsch feel free to merge. I'd prefer this on PGEngine, but also if we introduce another intermediate entity later we'll deprecate it from either location.

Do you mean as a class method? I have moved it to be an instance method for PGEngine because it needs the connection pool instantiated. I can add more tests as a fast follow.

@eyurtsev
Copy link
Collaborator

Do you mean as a class method? I have moved it to be an instance method for PGEngine because it needs the connection pool instantiated. I can add more tests as a fast follow.

Was thinking as an instance method that takes the table id as a parameter.

@averikitsch averikitsch merged commit 164810f into main Apr 15, 2025
5 checks passed
@averikitsch averikitsch deleted the drop-table branch April 15, 2025 23:05
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