Skip to content

Multiple Database Support#10

Open
rishipradeep-think41 wants to merge 4 commits into
think41:devfrom
rishipradeep-think41:feature/multi-db-support
Open

Multiple Database Support#10
rishipradeep-think41 wants to merge 4 commits into
think41:devfrom
rishipradeep-think41:feature/multi-db-support

Conversation

@rishipradeep-think41
Copy link
Copy Markdown

@rishipradeep-think41 rishipradeep-think41 commented Jun 8, 2025

Summary

This PR adds support for multiple database dialects in query generation, addressing think41/foundation-sql#4.

Note:

  • This PR is dependent on the changes from the schema lookup support PR - Schema Lookup support (PR) ,
  • Please review and merge that PR first. Once it is merged, this PR’s diff will only show the new changes related to dialect support. (Or merge this one and close the older one)

Related Issues

Changes

  • Implemented fetching of db_type param from db url
  • Modified system prompt to generate sql based on passed db_type param
  • Added db type to cached sql file names (eg: create_user_mysql.sql, create_user_postgres.sql)
  • Created a docker-compose file to create testing db instances
  • Added test files for mysql and postgres db testing
  • Added functionality in tests/utils.py to initialise db's before testing
  • Updated README to reflect testing changes
  • Minor code clean-up and formatting changes.

Formatting

  • Used the Black Python formatter for consistent code style across modified files.

Additional Notes

  • Please merge the schema lookup PR first.
  • Happy to rebase/update this PR as needed once the first PR is merged.

@anshum4n-git
Copy link
Copy Markdown
Collaborator

Can we scope down the changes - currently a lot of styling and files are changed across. However, focussing on just the important changes

  1. I have a similar comment in PR SQL Decorator created for TABLE_SCHEMA creation #11 but it might be a good idea to move the prompts to a jinja2 template. That way, the dialect is just an additional variable available and jinja if blocks can be used to get the correct prompts.
  2. Extract schema from DB - This by itself can be a separate PR. So two different smaller PRs are easier to get merged.
  3. Too many new dependencies are added. Think about removing some of them and adding some of them as packages e.g. foundation-sql[postgres], foundation-sql[mysql] etc. The user (developer) should be incharge of which optional dependencies they want to include. Some of that would also need to be modified in setup.py
  4. It would be interesting to think of the tests as multi-db tests i.e. we should be able to run the same BasicTest across mysql, sqlite, postgres and so on.

Overall, looks good. Would just want to break it into smaller change, avoid style only changes and improve on some of the above. Looking forward to getting this merged ... 👍

@rishipradeep-think41
Copy link
Copy Markdown
Author

  • Another pr for Schema Extraction from the DB has been raised before this one.
    Schema lookup implementation #9
  • Since this pr is dependent on that one, once it is merged, this PR’s diff will only show the new changes related to dialect support.
  • Will update to use jinja blocks to get the prompts instead and make dependecies optional

@anshum4n-git anshum4n-git changed the base branch from main to dev September 2, 2025 04:52
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.

Support multiple database dialects

3 participants