-
Notifications
You must be signed in to change notification settings - Fork 69
feat: add support of PGvector #1494
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
Conversation
wrapper/src/main/java/software/amazon/jdbc/PropertyDefinition.java
Outdated
Show resolved
Hide resolved
53caed8
to
a01aecb
Compare
|
||
Driver.skipWrappingForPackage("com.pgvector"); | ||
``` | ||
This feature can be used to allow the AWS Advanced JDBC Driver to properly handle popular database extensions like [PGvector](https://github.com/pgvector/pgvector-java) and [PostGIS](https://github.com/postgis/postgis-java). |
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.
Curious if we need to expand on the consequences of wrapping these packages? If there are any.
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.
If there are consequences it might be worth documenting them
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'm not sure what consequences it could be. Either the driver throw an exception since it doesn't have any suitable wrapper class, or it passes this classes intact. We can adjust the messages slightly:
This feature can be used to allow the AWS Advanced JDBC Driver to pass classes intact and, thus, allow support of popular database extensions like [PGvector](https://github.com/pgvector/pgvector-java) and [PostGIS](https://github.com/postgis/postgis-java).
How does it sound?
wrapper/src/main/java/software/amazon/jdbc/PropertyDefinition.java
Outdated
Show resolved
Hide resolved
wrapper/src/main/java/software/amazon/jdbc/ds/AwsWrapperDataSource.java
Outdated
Show resolved
Hide resolved
|
||
Driver.skipWrappingForPackage("com.pgvector"); | ||
``` | ||
This feature can be used to allow the AWS Advanced JDBC Driver to properly handle popular database extensions like [PGvector](https://github.com/pgvector/pgvector-java) and [PostGIS](https://github.com/postgis/postgis-java). |
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.
If there are consequences it might be worth documenting them
``` | ||
This feature can be used to allow the AWS Advanced JDBC Driver to properly handle popular database extensions like [PGvector](https://github.com/pgvector/pgvector-java) and [PostGIS](https://github.com/postgis/postgis-java). | ||
|
||
Using `Driver.skipWrappingForPackage()` method and using driver configuration parameter `skipWrappingForPackages` are functionally similar. The configuration parameter receives a comma separated list of package names while `Driver.skipWrappingForPackage()` accepts just one package at at time. |
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'm curious on the choices behind this. It seems there is 1 way for skipWrappingForType and 2 ways for skipWrappingForPackage. If the reason we introduced skipWrappingForPackages is to be easier than writing a line of code for each package - we should definitely have that for type as well as already in this example the number of types to skip is greater than the number of packages. Suggestion: either one way or two ways for both.
And, if we still want parameters..
i. should clarify whether or not skipWrappingForType has an equivalent?
ii. should have an example of using the parameters?
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 intention was to let a user a solution to provide an exceptional classes. The current solution with Driver.skipWrappingForType()
allows to do it with highest possible granularity. Some users may needs such precision due to high security requirements. A less granular and easier solution is to specify entire package name with Driver.skipWrappingForPackage()
. It allows more classes to be left intact so it could be considered as a less secure approach.
Both approaches above require a user to have access to source code of an application and that is not always the case. Many of out-of-box AI solutions provides a free access to pre-built versions, not to the sources. For such cases there's a configuration parameter skipWrappingForPackages
.
a01aecb
to
0ee6788
Compare
Summary
Allow user to specify classes and java packages that don't require wrapping.
Specifying
PGvector
orPostGIS
classes could be a good example of use.#1399
Description
Additional Reviewers
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.