feat: integration for pg_handler added #937
Conversation
gaurav274
commented
Aug 17, 2023
gaurav274
commented
- Added Abstract Class for database integrations.
- Implemented postgres integration
|
@gaurav274 Can you give a quick read on this? |
LGTM |
| @@ -0,0 +1,112 @@ | |||
| # coding=utf-8 | |||
There was a problem hiding this comment.
Since this is in the same folder as types.py and interface.py, shall we move this to a separate dir
evadb/third_party/databases/handlers/postgres_handler/.....
This would also make it cleaner while adding handlers for other backends
There was a problem hiding this comment.
The file structure is inspired by https://github.com/mindsdb/mindsdb/tree/staging/mindsdb/integrations/handlers
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
| import pandas as pd | ||
| import psycopg2 |
There was a problem hiding this comment.
I dont think this will be installed by default in circleci since its under a separate header in setup.py
There was a problem hiding this comment.
Yes. We need to fix import psycopg2 across the code base. We should only import this if user is creating a postgres database connection.
There was a problem hiding this comment.
Add dynamic install and import here 106896b.
I will work on another PR to allow testing everything in CI.
| @@ -0,0 +1,2 @@ | |||
| psycopg<=2.9.7 | |||
| pandas<=2.0.2 | |||
There was a problem hiding this comment.
Is there a reason we want to have a cap on the version? This very likely leads to version conflicts, given postgres is not installed with the evadb. From my experience with the current setup.py, pip install -e .[dev,some_other_dependency] works, but pip install -e .[dev] and then pip install -e .[some_other_dependency] will print out version conflict warnings/errors, especially when we have version caps on some packages.
gaurav274
added
Feature Request ✨
