community/SQLDatabase: Fetch mode `cursor`, query parameters, query by selectable, expose execution options, and documentation #16655

amotl · 2024-01-26T23:53:39Z

Description: Improve SQLDatabase adapter component to promote code re-use, see suggestion.
Needed by: community: Add SQLDatabaseLoader document loader #16246
Addressed to: @baskaryan, @cbornet

Details

Add cursor fetch mode
Accept SQL query parameters
Accept both str and SQLAlchemy selectables as query expression
Expose execution_options
Documentation page (notebook) about SQLDatabase ¹
See About SQLDatabase.

Apparently there hasn't been any yet? ↩

vercel · 2024-01-26T23:53:42Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
langchain	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 5, 2024 8:41pm

libs/langchain/tests/unit_tests/test_sql_database.py

libs/community/langchain_community/utilities/sql_database.py

baskaryan · 2024-01-29T21:52:58Z

libs/core/langchain_core/callbacks/manager.py

why is this needed? feels like the on_text method should really just accept strings

Hi @baskaryan. This adjustment was needed to satisfy the mypy linter, to accompany the possibility that this can now also return a SQLAlchemy Result object here, which would otherwise fail.

Can we just cast the output of SQLDatabase wherever it's passed to on_text instead of changing the on_text method (since it doesn't actually support non-strings)?

Hi again. I just reverted this adjustment, and added a cast at a different spot like you suggested, on behalf of 08db71b (now squashed).

Edit: Looks good so far, the current linter failures on CI are unrelated, as they also appear on a run of a different PR.

After rebasing this patch on top of master, it needed a few adjustments, which have been implemented now. Based on the CI outcome, the patch may receive another round of code reviews. Thank you.

Hi again. It looks like @eyurtsev is taking over. Thank you so much!

libs/experimental/langchain_experimental/sql/vector_sql.py

libs/community/langchain_community/utilities/sql_database.py

…ctable - Add `cursor` fetch mode - Make `_execute` accept query parameters - Accept both `str` or SQLAlchemy selectables for querying

Up until now, the `SQLDatabase` utility only has been demonstrated on behalf of the "toolkit integration" section, where it is about showcasing an agent designed to interact with an SQL database. [1] Contrary to that, this patch adds a documentation page exclusively dedicated to the SQLDatabase adapter utility, and its features. [1] https://python.langchain.com/docs/integrations/toolkits/sql_database

amotl · 2024-02-05T21:36:18Z

libs/experimental/langchain_experimental/sql/base.py

-            _run_manager.on_text(result, color="yellow", verbose=self.verbose)
+            _run_manager.on_text(str(result), color="yellow", verbose=self.verbose)


Can we just cast the output of SQLDatabase wherever it's passed to on_text instead of changing the on_text method.

@baskaryan: Your suggestion from #16655 (comment) has been implemented here. mypy is fine with that, and we do not need to touch anything in core.

This seems fine to me

amotl · 2024-02-06T02:46:21Z

libs/langchain/tests/unit_tests/test_sql_database.py

I am wondering if this unit test file, together with test_sql_database_schema.py, should actually move into community/tests/unit_tests, following the logic of recent refactorings, and given that the files under test are also in community?

Probably yes, we can follow up in a separate PR

eyurtsev · 2024-02-07T18:52:27Z

Reviewing and comandeering to adjust a few things in the API

eyurtsev · 2024-02-07T19:38:55Z

@amotl are you able to give me permissions to modify the head on the given branch? (it's an option you can enable when creating the PR)

I fixed a breaking change due to lack of * syntax, and updated the notebook with a security notebook. We can merge afterwards.

I left another review on the original PR to see if keyset pagination is an option (instead of relying on a cursor)

eyurtsev · 2024-02-07T19:41:42Z

Here is a PR that contains the proposed changes: https://github.com/langchain-ai/langchain/pull/17191/files, would prefer to push here so you can 👍 👎 the differences

amotl · 2024-02-07T21:20:18Z

Hi @eyurtsev. Thanks for the review.

are you able to give me permissions to modify the head on the given branch? (it's an option you can enable when creating the PR)

Is there a way to modify it on the PR afterwards? I admit I never recognized that option, apologies. In any case, I've added you to the list of contributors, where you may be able to accept it on the invitations page. Does that help already?

Here is a PR that contains the proposed changes: https://github.com/langchain-ai/langchain/pull/17191/files, would prefer to push here so you can 👍 👎 the differences

Thanks. I've looked at 4f3b063 and 806ba2e, and I appreciate your improvements. Now, I hope we will get the permission problem sorted out?

amotl · 2024-02-07T21:29:59Z

I hope we will get the permission problem sorted out?

I've just digested the documentation about Allowing changes to a pull request branch created from a fork, but I don't see the Allow edits from maintainers. or Allow edits and access to secrets by maintainers. options on the PR.

Within the documentation, they refer to "forks owned by a personal account". Is that the reason it doesn't work, because this fork is on an organization account?

amotl · 2024-02-07T21:33:45Z

Is that the reason it doesn't work, because this fork is on an organization account?

Right, here we go -- it does not work:

You cannot give push permissions to a fork owned by an organization.

-- https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/about-permissions-and-visibility-of-forks#about-permissions-of-forks

What I was able to do is to add you to the list of contributors, where you may be able to accept it on the invitations page.

Does that help you in any other way?

eyurtsev · 2024-02-08T03:22:52Z

@amotl I'll close this PR and merge the branch I had: #17191 hope this is OK

eyurtsev · 2024-02-08T03:25:15Z

Merged the other PR in. I'll read up on the links you sent to try and figure out if there's any other way to do this -- from what i understood it sounds like there's a difference between forks owned by individuals vs. forks owned by orgs

amotl · 2024-02-08T13:35:06Z

Merged the other PR in. Hope this is OK.

Definitively, I appreciate it. Thanks!

from what i understood it sounds like there's a difference between forks owned by individuals vs. forks owned by orgs

Exactly. Apparently, repos owned by orgs do not offer that feature to permit upstream authors access to their PRs. Bummer!

@baskaryan

- **Description:** Improve test cases for `SQLDatabase` adapter component, see [suggestion](#16655 (review)). - **Depends on:** GH-16655 - **Addressed to:** @baskaryan, @cbornet, @eyurtsev _Remark: This PR is stacked upon GH-16655, so that one will need to go in first._ Edit: Thank you for bringing in GH-17191, @eyurtsev. This is a little aftermath, improving/streamlining the corresponding test cases.

@baskaryan

…ai#16659) - **Description:** Improve test cases for `SQLDatabase` adapter component, see [suggestion](langchain-ai#16655 (review)). - **Depends on:** langchain-aiGH-16655 - **Addressed to:** @baskaryan, @cbornet, @eyurtsev _Remark: This PR is stacked upon langchain-aiGH-16655, so that one will need to go in first._ Edit: Thank you for bringing in langchain-aiGH-17191, @eyurtsev. This is a little aftermath, improving/streamlining the corresponding test cases.

@baskaryan

…ai#16659) - **Description:** Improve test cases for `SQLDatabase` adapter component, see [suggestion](langchain-ai#16655 (review)). - **Depends on:** langchain-aiGH-16655 - **Addressed to:** @baskaryan, @cbornet, @eyurtsev _Remark: This PR is stacked upon langchain-aiGH-16655, so that one will need to go in first._ Edit: Thank you for bringing in langchain-aiGH-17191, @eyurtsev. This is a little aftermath, improving/streamlining the corresponding test cases.

@baskaryan

- **Description:** A generic document loader adapter for SQLAlchemy on top of LangChain's `SQLDatabaseLoader`. - **Needed by:** crate-workbench#1 - **Depends on:** GH-16655 - **Addressed to:** @baskaryan, @cbornet, @eyurtsev Hi from CrateDB again, in the same spirit like GH-16243 and GH-16244, this patch breaks out another commit from crate-workbench#1, in order to reduce the size of this patch before submitting it, and to separate concerns. To accompany the SQLAlchemy adapter implementation, the patch includes integration tests for both SQLite and PostgreSQL. Let me know if corresponding utility resources should be added at different spots. With kind regards, Andreas. ### Software Tests ```console docker compose --file libs/community/tests/integration_tests/document_loaders/docker-compose/postgresql.yml up ``` ```console cd libs/community pip install psycopg2-binary pytest -vvv tests/integration_tests -k sqldatabase ``` ``` 14 passed ``` ![image](https://github.com/langchain-ai/langchain/assets/453543/42be233c-eb37-4c76-a830-474276e01436) --------- Co-authored-by: Andreas Motl <andreas.motl@crate.io>

@baskaryan

…ai#18281) - **Description:** A generic document loader adapter for SQLAlchemy on top of LangChain's `SQLDatabaseLoader`. - **Needed by:** crate-workbench#1 - **Depends on:** langchain-aiGH-16655 - **Addressed to:** @baskaryan, @cbornet, @eyurtsev Hi from CrateDB again, in the same spirit like langchain-aiGH-16243 and langchain-aiGH-16244, this patch breaks out another commit from crate-workbench#1, in order to reduce the size of this patch before submitting it, and to separate concerns. To accompany the SQLAlchemy adapter implementation, the patch includes integration tests for both SQLite and PostgreSQL. Let me know if corresponding utility resources should be added at different spots. With kind regards, Andreas. ### Software Tests ```console docker compose --file libs/community/tests/integration_tests/document_loaders/docker-compose/postgresql.yml up ``` ```console cd libs/community pip install psycopg2-binary pytest -vvv tests/integration_tests -k sqldatabase ``` ``` 14 passed ``` ![image](https://github.com/langchain-ai/langchain/assets/453543/42be233c-eb37-4c76-a830-474276e01436) --------- Co-authored-by: Andreas Motl <andreas.motl@crate.io>

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. 🤖:improvement Medium size change to existing code to handle new use-cases labels Jan 26, 2024

amotl force-pushed the sqldatabase-improvements branch from 219e0a9 to ef7805b Compare January 27, 2024 00:00

amotl mentioned this pull request Jan 27, 2024

community: Add SQLDatabaseLoader document loader #16246

Closed

amotl force-pushed the sqldatabase-improvements branch from ef7805b to 45649a8 Compare January 27, 2024 00:08

amotl commented Jan 27, 2024

View reviewed changes

libs/langchain/tests/unit_tests/test_sql_database.py Outdated Show resolved Hide resolved

amotl force-pushed the sqldatabase-improvements branch 2 times, most recently from 916e376 to 194d257 Compare January 27, 2024 00:15

amotl marked this pull request as draft January 27, 2024 00:16

amotl force-pushed the sqldatabase-improvements branch 2 times, most recently from b3df1b5 to 9cb633b Compare January 27, 2024 00:22

amotl marked this pull request as ready for review January 27, 2024 00:26

amotl commented Jan 27, 2024

View reviewed changes

libs/community/langchain_community/utilities/sql_database.py Outdated Show resolved Hide resolved

libs/community/langchain_community/utilities/sql_database.py Show resolved Hide resolved

libs/community/langchain_community/utilities/sql_database.py Show resolved Hide resolved

amotl mentioned this pull request Jan 27, 2024

community/SQLDatabase: Generalize and trim software tests #16659

Merged

amotl force-pushed the sqldatabase-improvements branch 2 times, most recently from b62229c to 96cfc1f Compare January 27, 2024 17:46

vercel bot deployed to Preview January 27, 2024 17:57 View deployment

cbornet reviewed Jan 27, 2024

View reviewed changes

libs/community/langchain_community/utilities/sql_database.py Outdated Show resolved Hide resolved

dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jan 27, 2024

amotl requested a review from cbornet January 27, 2024 20:26

vercel bot deployed to Preview January 27, 2024 20:31 View deployment

amotl changed the title ~~community/SQLDatabase: Fetch mode, query parameters, query by selectable~~ community/SQLDatabase: Fetch mode, query parameters, query by selectable, expose execution options, and documentation Jan 27, 2024

cbornet mentioned this pull request Jan 29, 2024

community: allow SQLDatabase to run queries with parameters #15453

Closed

baskaryan reviewed Jan 29, 2024

View reviewed changes

libs/experimental/langchain_experimental/sql/vector_sql.py Show resolved Hide resolved

baskaryan reviewed Jan 29, 2024

View reviewed changes

libs/community/langchain_community/utilities/sql_database.py Outdated Show resolved Hide resolved

amotl requested a review from baskaryan January 29, 2024 23:32

amotl changed the title ~~community/SQLDatabase: Fetch mode, query parameters, query by selectable, expose execution options, and documentation~~ community/SQLDatabase: Fetch mode cursor, query parameters, query by selectable, expose execution options, and documentation Jan 30, 2024

amotl force-pushed the sqldatabase-improvements branch from 1ea8e4d to d3b6d59 Compare February 5, 2024 19:26

vercel bot deployed to Preview February 5, 2024 19:34 View deployment

amotl force-pushed the sqldatabase-improvements branch from d3b6d59 to 8003fb3 Compare February 5, 2024 19:43

vercel bot deployed to Preview February 5, 2024 19:50 View deployment

amotl added 2 commits February 5, 2024 21:33

community/SQLDatabase: Fetch mode cursor, parameters, query by sele…

fb5adcd

…ctable - Add `cursor` fetch mode - Make `_execute` accept query parameters - Accept both `str` or SQLAlchemy selectables for querying

amotl force-pushed the sqldatabase-improvements branch from 8003fb3 to e4d2f3e Compare February 5, 2024 20:33

vercel bot deployed to Preview February 5, 2024 20:41 View deployment

amotl commented Feb 5, 2024

View reviewed changes

amotl commented Feb 6, 2024

View reviewed changes

eyurtsev self-assigned this Feb 7, 2024

eyurtsev added the waiting-on-author PR Status: Confirmation from author is required label Feb 7, 2024

amotl requested a review from eyurtsev February 7, 2024 21:20

eyurtsev mentioned this pull request Feb 8, 2024

community[minor]: SQLDatabase Add fetch mode cursor, query parameters, query by selectable, expose execution options, and documentation #17191

Merged

eyurtsev closed this Feb 8, 2024

eyurtsev mentioned this pull request Feb 28, 2024

community[minor]: Add SQLDatabaseLoader document loader #18281

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

community/SQLDatabase: Fetch mode `cursor`, query parameters, query by selectable, expose execution options, and documentation #16655

community/SQLDatabase: Fetch mode `cursor`, query parameters, query by selectable, expose execution options, and documentation #16655

amotl commented Jan 26, 2024 •

edited

Loading

vercel bot commented Jan 26, 2024 •

edited

Loading

baskaryan Jan 29, 2024

amotl Jan 29, 2024 •

edited

Loading

amotl Feb 5, 2024 •

edited

Loading

amotl Feb 5, 2024

amotl Feb 7, 2024

amotl Feb 5, 2024 •

edited

Loading

eyurtsev Feb 7, 2024

amotl Feb 6, 2024 •

edited

Loading

eyurtsev Feb 7, 2024

eyurtsev commented Feb 7, 2024

eyurtsev commented Feb 7, 2024

eyurtsev commented Feb 7, 2024

amotl commented Feb 7, 2024

amotl commented Feb 7, 2024

amotl commented Feb 7, 2024 •

edited

Loading

eyurtsev commented Feb 8, 2024

eyurtsev commented Feb 8, 2024

amotl commented Feb 8, 2024 •

edited

Loading

		_run_manager.on_text(result, color="yellow", verbose=self.verbose)
		_run_manager.on_text(str(result), color="yellow", verbose=self.verbose)

community/SQLDatabase: Fetch mode cursor, query parameters, query by selectable, expose execution options, and documentation #16655

community/SQLDatabase: Fetch mode cursor, query parameters, query by selectable, expose execution options, and documentation #16655

Conversation

amotl commented Jan 26, 2024 • edited Loading

Details

Footnotes

vercel bot commented Jan 26, 2024 • edited Loading

baskaryan Jan 29, 2024

Choose a reason for hiding this comment

amotl Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

amotl Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

amotl Feb 5, 2024

Choose a reason for hiding this comment

amotl Feb 7, 2024

Choose a reason for hiding this comment

amotl Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

eyurtsev Feb 7, 2024

Choose a reason for hiding this comment

amotl Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

eyurtsev Feb 7, 2024

Choose a reason for hiding this comment

eyurtsev commented Feb 7, 2024

eyurtsev commented Feb 7, 2024

eyurtsev commented Feb 7, 2024

amotl commented Feb 7, 2024

amotl commented Feb 7, 2024

amotl commented Feb 7, 2024 • edited Loading

eyurtsev commented Feb 8, 2024

eyurtsev commented Feb 8, 2024

amotl commented Feb 8, 2024 • edited Loading

community/SQLDatabase: Fetch mode `cursor`, query parameters, query by selectable, expose execution options, and documentation #16655

community/SQLDatabase: Fetch mode `cursor`, query parameters, query by selectable, expose execution options, and documentation #16655

amotl commented Jan 26, 2024 •

edited

Loading

vercel bot commented Jan 26, 2024 •

edited

Loading

amotl Jan 29, 2024 •

edited

Loading

amotl Feb 5, 2024 •

edited

Loading

amotl Feb 5, 2024 •

edited

Loading

amotl Feb 6, 2024 •

edited

Loading

amotl commented Feb 7, 2024 •

edited

Loading

amotl commented Feb 8, 2024 •

edited

Loading