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.

Sign up for GitHub

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

Jump to bottom

community: allow SQLDatabase to run queries with parameters #15453

Closed
wants to merge 1 commit into from
Closed

community: allow SQLDatabase to run queries with parameters #15453

wants to merge 1 commit into from

Conversation

danb27
Copy link
Contributor

@danb27 danb27 commented Jan 3, 2024

Description: Allow binding parameters in queries run by SQLDatabase
Issue: #15449
Dependencies: N/A
Twitter handle: N/A

Let me know if you need me to write tests, it looks like this class doesn't have any tests.

@vercel Vercel
Copy link

vercel bot commented Jan 3, 2024 •
edited
Loading

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Jan 3, 2024 7:24am

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. Ɑ: langserve Related to LangServe package 🤖:improvement Medium size change to existing code to handle new use-cases labels Jan 3, 2024
@danb27
Copy link
Contributor Author

danb27 commented Jan 8, 2024

Hello, @eyurtsev

Been a few days - just bumping this up. Thanks!

@baskaryan
Copy link
Collaborator

cc @amotl @cbornet i know you're working on something similar

@cbornet
Copy link
Collaborator

cbornet commented Jan 29, 2024

cc @amotl @cbornet i know you're working on something similar

Indeed I think #16655 has the same change

@amotl
Copy link
Contributor

amotl commented Jan 29, 2024 •
edited
Loading

Hi. Yeah I guess GH-16655 implements this already, and more, including test cases. I think the patch is ready, so may I ask someone with corresponding powers merge it?

Thanks @danb27 for also tackling this. Are you comfortable with giving my patch precedence? If it is not appropriate, please let me know and I will rebase my patch on top of yours. Thanks!

@hwchase17 hwchase17 closed this Jan 30, 2024
@baskaryan baskaryan reopened this Jan 30, 2024
@danb27
Copy link
Contributor Author

danb27 commented Feb 21, 2024

Hi. Yeah I guess GH-16655 implements this already, and more, including test cases. I think the patch is ready, so may I ask someone with corresponding powers merge it?

Thanks @danb27 for also tackling this. Are you comfortable with giving my patch precedence? If it is not appropriate, please let me know and I will rebase my patch on top of yours. Thanks!

Wow, sorry I missed this! Would love to contribute but looks like your MR has many other features as well. Either way!

@amotl
Copy link
Contributor

amotl commented Feb 22, 2024

Hi Dan. Thanks for your reply. This patch has now been merged already by @eyurtsev. Does it fulfill your needs already?

@danb27
Copy link
Contributor Author

danb27 commented Feb 22, 2024

Yup! Sure does.

@amotl
Copy link
Contributor

amotl commented Feb 22, 2024

Excellent, thanks, and enjoy! If you think it is appropriate to close this PR, please do, if @baskaryan or @cbornet don't have any objections.

@danb27 danb27 closed this Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
🤖:improvement Medium size change to existing code to handle new use-cases Ɑ: langserve Related to LangServe package size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@danb27 @baskaryan @cbornet @amotl @hwchase17