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.

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

fix: strip trailing semicolon in cosmosdb and unify implementation #514

Closed
wants to merge 1 commit into from

Conversation

murfffi
Copy link
Contributor

@murfffi murfffi commented Mar 1, 2025

Fixes, in part, #511 .

cosmosdb, like several other databases, needs trailing semicolons to be
removed from the statement before it is passed to the driver.

This fix applies that to cosmosdb and makes the other databases with identical
processing use the same implementation - presto, trino, athena, SAP ASE.

Drivers that strip trailing semicolon as part of other processing are
not included in the refactoring to keep scope small.

Testing: with xo/dburl#44 applied , testing cosmosdb ...

image

Also tested with trino.

@murfffi murfffi changed the title Semicoloncosmos fix: strip trailing semicolon in cosmosdb and unify implementation Mar 1, 2025
@kenshaw
Copy link
Member

kenshaw commented Mar 4, 2025

Please drop the gocosmos alias change, and rebase this as a single commit. I'll rebase this on master and back-port this to the release-19 branch and cut a release. I'll also update the dburl dependency on the release-19 branch at that time, so the gocosmos alias works properly. Thanks.

cosmosdb, like several other databases, needs trailing semicolons to be
removed from the statement before it is passed to the driver.

This fix applies that to cosmosdb and makes the other databases with identical
processing use the same implementation - presto, trino, athena, SAP ASE.

Drivers that strip trailing semicolon as part of other processing are
not included in the refactoring to keep scope small.

Testing: will run usql with trino, and cosmosdb
@murfffi
Copy link
Contributor Author

murfffi commented Mar 4, 2025

Thanks for taking a look! Squashed the commits. I left the gocosmos alias in because the two argument form of \connect - with native DSN - needs that alias to work:

image

Without native DSN \connect, users need to url-encode the cosmos API key so dburl can parse the connection string properly. I could add the alias as a separate PR but that will require even more work merging to release-19.

@murfffi
Copy link
Contributor Author

murfffi commented Mar 4, 2025

Tested trino also to confirm that the mild refactoring didn't break the other affected DBs.

@kenshaw kenshaw closed this Mar 5, 2025
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.

2 participants