-
Notifications
You must be signed in to change notification settings - Fork 32
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
Feat: add support for initializing vecs client with custom schema #63
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice job on the PR
did you see this comment here:
#62 (comment)
if we add a top level parameter for schema in the Client
it'll be much more difficult to support defining a schema on the Collection
when that inevitably gets requested.
Would you be up for refactoring to make this a per Collection
keyword only argument?
Note that it'd be a bit more involved since the Metadata
instance can't be bound to a single schema in that case
@olirice thanks for the feedback! sure, I'm definitely up to implement that change. |
update - move support for schema onto collection instead of client
hey @olirice, made some updates. let me know your thoughts, thanks! |
looks great. if we can address those last few concerns this will merge |
I'm looking forward to this PR being merged! |
hey, thanks @olirice for the review! will finish this up this week |
updates based on feedback
Creates a client from a Postgres connection string and optional schema. | ||
Defaults to `vecs` schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and optional schema. Defaults to `vecs` schema.
can be removed
- id: black | ||
language_version: python3.9 | ||
language_version: python3.8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the rationale for this change?
@@ -13,6 +13,7 @@ | |||
import vecs | |||
|
|||
PYTEST_DB = "postgresql://postgres:password@localhost:5611/vecs_db" | |||
PYTEST_SCHEMA = "test_schema" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the best way to test that escaping is done correctly in all places is to use a crazy schema name.
I tested basic operations with the schema name "esCape Me!" and the test suite fails.
To reproduce that, try:
foo: Collection = client.get_or_create_collection(name="foo", schema="esCape Me!", dimension=5)
and you'll get
sqlalchemy.exc.ProgrammingError: (psycopg2.errors.InvalidSchemaName) schema ""esCape Me!"" does not exist
What kind of change does this PR introduce?
Feature: add support for connecting to a user-specified database schema (instead of hard-coding
vecs
).This was accomplished by adding the
schema
argument to the Client class and replacing all hard-coded references to schemavecs
.Also added
schema
parameter tocreate_client
method. For backwards compatibility, it still defaults tovecs
.What is the current behavior?
Currently, the client defaults to schema
vecs
. Users may want to place vecs collections under a schema according to their own naming conventions.Addresses #62
Please link any relevant issues here.
What is the new behavior?
Users can now instantiate a Client instance with a custom schema. For example:
Additional context
Add any other context or screenshots.
Test coverage attached
![Screen Shot 2023-12-06 at 9 32 26 PM](https://private-user-images.githubusercontent.com/38793515/288607680-4a11356b-0153-45fa-bb71-488233d7af34.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ1MjYxNzEsIm5iZiI6MTcyNDUyNTg3MSwicGF0aCI6Ii8zODc5MzUxNS8yODg2MDc2ODAtNGExMTM1NmItMDE1My00NWZhLWJiNzEtNDg4MjMzZDdhZjM0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjQlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODI0VDE4NTc1MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTE0OWFiZDk1ODJjMTFmYWE1Y2UzMGQ2MGM1ZDBmNzUxMzUwMGVlMTcwYmI5ZjA4YzZlY2UyNWY0YzBjNDFlY2MmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.p-mqlKaOTD8Ot5XDNlreUCVHFuuFrCuDYAEtBFvOY9Y)