-
Notifications
You must be signed in to change notification settings - Fork 223
Feat/postgres schema support #1138
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/postgres schema support #1138
Conversation
please let me know for any changes thanks!! |
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.
Can you also help with updating this doc: https://github.com/cocoindex-io/cocoindex/blob/main/docs/docs/sources/index.md?plain=1
Thanks!
src/ops/targets/postgres.rs
Outdated
fn qualified_table_name(table_id: &TableId) -> String { | ||
match &table_id.schema { | ||
Some(schema) => format!("\"{}\".\"{}\"", schema, table_id.table_name), | ||
None => format!("\"{}\"", table_id.table_name), |
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.
Let's leave table name unquoted for this case for now. Only double-qutes schema names.
We considered adding double-quotes for table names before, but didn't proceed because it'll break existing pipelines. We'll make this change in our next major version upgrade (e.g. v1) which is considered to be backward imcompatible.
src/ops/targets/postgres.rs
Outdated
// Create schema if specified | ||
if let Some(schema) = &table_id.schema { | ||
let sql = format!("CREATE SCHEMA IF NOT EXISTS \"{}\"", schema); | ||
sqlx::query(&sql).execute(db_pool).await?; | ||
} | ||
|
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.
We only need to do it when we need to run CREATE TABLE ...
. We can move it before executing that statement.
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.
okay got it
Can you also describe how you test it end to end? Thanks! |
thanks! I tested this feature with local postgres server:
for database verification:
|
pub struct Spec { | ||
database: Option<spec::AuthEntryReference<DatabaseConnectionSpec>>, | ||
table_name: Option<String>, | ||
schema: Option<String>, |
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.
I think at least the field also needs to be added to this dataclass in Python SDK:
cocoindex/python/cocoindex/targets/_engine_builtin_specs.py
Lines 12 to 16 in afea19d
class Postgres(op.TargetSpec): | |
"""Target powered by Postgres and pgvector.""" | |
database: AuthEntryReference[DatabaseConnectionSpec] | None = None | |
table_name: str | None = None |
So that users will be able to use it.
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.
okay will do
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.
database: AuthEntryReference[DatabaseConnectionSpec] | None = None
table_name: str | None = None
schema: str | None = None
I picked text-embedding example and put the schema then ran the setup nd update cmd |
Thanks! Looks good! Can you also help update the doc in the location above? Thanks! |
yes |
what should I include also do I need to update target file? some pointers |
Sorry I gave the wrong link. It should be for the target, not for the source: https://github.com/cocoindex-io/cocoindex/blob/main/docs/docs/targets/postgres.md?plain=1 The description for new |
Looks good! Thanks for the PR. Merging. |
fixes #722
feature implemented ->