Fix problem with sql_chain and quotation marks: normalize_sql_cmd

[PhilipMay]

fixes #3305

[PhilipMay]

I am not 100% sure if this locale solution (@staticmethod in SQLDatabaseChain) is the right solution.

Are there more places where this kind of normalization is needed or even implemented? Should that normalization
be on a more generic place in the code?

I do not know. Need help from maintainers here...

[PhilipMay]

This is ready for review please.

[bllchmbrs]

@PhilipMay , I hit this issue too.

One problem that might come up I believe with this implementation is the following query.

"SELECT * FROM blah_table where x='F'

I'm not sure if it's more likely that quotations aren't mirrored (e.g., starts with " ends with ') or whether it's more likely that a quotation would be omitted (like in my above example). If I read your implementation right, it remove the final ' from the query, even though it should keep it.

Not sure of the right answer here, but wanted to bring it up to make sure we're covering all the edge cases effectively.

[PhilipMay]

Hi @anabranch

If I read your implementation right, it remove the final ' from the query, even though it should keep it.

That is not right. The code below sais:

"(if it starts and ends with double quotation mark) or (if it starts and ends with single quotation mark)"

If it starts with double and ends with single -> nothing will be changed. IMO that also would be incorrect SQL.

            if (
                (sql_cmd.startswith('"') and sql_cmd.endswith('"'))
                or (sql_cmd.startswith("'") and sql_cmd.endswith("'"))
            ) and (len(sql_cmd) >= 2):

Does that resolve your reservations? @anabranch

[PhilipMay]

@anabranch I added a test for this case:

https://github.com/telekom/langchain/blob/738df5ce20ec75d30af166580d8a1318895370c4/tests/unit_tests/chains/test_sql_database.py#L33

[PhilipMay]

@hwchase17 can you please approve the workflows?

[PhilipMay]

@hwchase17 can you please approve the workflows?

Thanks.

Ok. So checks are green. IMO this can be merged. :-)

[hansvdam]

I'm not sure, but to me the problem is that the prompt template is wrong in asking for an SQL-statement with quotes. I think it is better to just not do that:
#4101

[PhilipMay]

I'm not sure, but to me the problem is that the prompt template is wrong in asking for an SQL-statement with quotes. I think it is better to just not do that: #4101

I agree. #4101 would be useful. But removing the quotes - just in case - would also be useful IMO.

[leo-gan]

@PhilipMay Hi , could you, please, resolve the merging issues and address the last comments (if needed)? After that, ping me and I push this PR for the review. Thanks!

If this PR is not needed anymore, could you, please, let me know?

[efriis]

Closing because the PR wouldn't line up with the current directory structure of the library (would need to be in /libs/langchain/langchain instead of /langchain). Feel free to reopen against the current head if it's still relevant!