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

vectorstores: add pgvector #377

Merged
merged 1 commit into from
Dec 13, 2023
Merged

vectorstores: add pgvector #377

merged 1 commit into from
Dec 13, 2023

Conversation

Abirdcfly
Copy link
Contributor

PR Checklist

  • Read the Contributing documentation.
  • Read the Code of conduct documentation.
  • Name your Pull Request title clearly, concisely, and prefixed with the name of the primarily affected package you changed according to Good commit messages (such as memory: add interfaces for X, Y or util: add whizzbang helpers).
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. Fixes #123).
  • Describes the source of new concepts.
  • References existing implementations as appropriate.
  • Contains test coverage for new functions.
  • Passes all golangci-lint checks.

@Abirdcfly Abirdcfly marked this pull request as ready for review December 1, 2023 13:59
if _, err = tx.Exec(ctx, "SELECT pg_advisory_xact_lock(1573678846307946494)"); err != nil {
return err
}
sql := fmt.Sprintf(`CREATE TABLE IF NOT EXISTS %s (
Copy link
Owner

Choose a reason for hiding this comment

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

rather than string-assemble this can you use arguments to Exec (throughout) -- this protects against sql injection attacks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, according to https://www.postgresql.org/docs/current/xfunc-sql.html#XFUNC-SQL-FUNCTION-ARGUMENTS: SQL function arguments can only be used as data values, not as identifiers. replacing the table name with $1 does not seem to work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Travis maybe means that you shouldn't Sprintf into package sql calls like Exec, but rather use the args of each method, e.g. look at the example at https://pkg.go.dev/database/sql#example-Conn.ExecContext

result, err := conn.ExecContext(ctx,UPDATE balances SET balance = balance + 10 WHERE user_id = ?;, id)

See how id is passed into the SQL query?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes that's what I meant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I may be getting it, but it seems difficult to do this if we wants to keep the table name as a parameter create table ? or create table $1 will both report an error, and I have to use the Sprint function and do Sanitize() when the user passes in the table name.

Or, we can fix the table name to be a constant and unchangeable.

Everywhere else, $1 is used to replace the parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

kindly ping @eliben @tmc

@tmc
Copy link
Owner

tmc commented Dec 1, 2023

Please add/include an example showing usage.

vectorstores/pgvector/doc.go Outdated Show resolved Hide resolved
vectorstores/pgvector/pgvector.go Outdated Show resolved Hide resolved
vectorstores/pgvector/pgvector.go Outdated Show resolved Hide resolved
vectorstores/pgvector/pgvector.go Outdated Show resolved Hide resolved
vectorstores/pgvector/pgvector.go Outdated Show resolved Hide resolved
vectorstores/pgvector/pgvector.go Show resolved Hide resolved
vectorstores/pgvector/pgvector.go Show resolved Hide resolved
vectorstores/pgvector/pgvector.go Show resolved Hide resolved
vectorstores/pgvector/pgvector_test.go Show resolved Hide resolved
@Abirdcfly
Copy link
Contributor Author

Please add/include an example showing usage.

Should I refer to the vectorstores/pgvector/pgvector_test.go file in examples dir to write an example of storing vectors using openai llm and pgvector? 🤔

if _, err = tx.Exec(ctx, "SELECT pg_advisory_xact_lock(1573678846307946494)"); err != nil {
return err
}
sql := fmt.Sprintf(`CREATE TABLE IF NOT EXISTS %s (
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Travis maybe means that you shouldn't Sprintf into package sql calls like Exec, but rather use the args of each method, e.g. look at the example at https://pkg.go.dev/database/sql#example-Conn.ExecContext

result, err := conn.ExecContext(ctx,UPDATE balances SET balance = balance + 10 WHERE user_id = ?;, id)

See how id is passed into the SQL query?

vectorstores/pgvector/pgvector.go Show resolved Hide resolved
Copy link
Collaborator

@eliben eliben left a comment

Choose a reason for hiding this comment

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

I think my review comments have been addressed

On to @tmc to approve and merge if he's happy with the PR

Signed-off-by: Abirdcfly <fp544037857@gmail.com>
Copy link
Owner

@tmc tmc left a comment

Choose a reason for hiding this comment

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

LGTM!

@tmc tmc merged commit ed0536f into tmc:main Dec 13, 2023
3 checks passed
@tmc tmc mentioned this pull request Dec 13, 2023
@tmc
Copy link
Owner

tmc commented Dec 13, 2023

Please add/include an example showing usage.

Should I refer to the vectorstores/pgvector/pgvector_test.go file in examples dir to write an example of storing vectors using openai llm and pgvector? 🤔

That would work, I'd love to see more ollama examples but you can you openai, up to you.

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