sqlx icon indicating copy to clipboard operation
sqlx copied to clipboard

added NextResultSet implementation

Open kmpm opened this issue 4 years ago • 11 comments

Trying to revive the solution for issue #380

Shamelessly used the stuff that @basvanbeek created in pr #453 but added some testing with postgres. This would also work with mssql but I didn't want to mess with the test environment.

kmpm avatar Jun 21 '21 09:06 kmpm

@jmoiron, what is the preferred way of getting something like this included?

kmpm avatar Sep 06 '22 05:09 kmpm

This project appears to have been abandoned I've merged this into my own fork here https://github.com/BenKnigge/sqly If anyone else would be interested in going through this pr black log and merging them into my fork the help would be welcomed.

BenKnigge avatar Aug 05 '23 11:08 BenKnigge

This project appears to have been abandoned I've merged this into my own fork here https://github.com/BenKnigge/sqly If anyone else would be interested in going through this pr black log and merging them into my fork the help would be welcomed.

@BenKnigge Should we use your fork or https://github.com/go-sqlx/sqlx? I recall seeing your name link to that fork as well in another issue or PR comment.

tazimmerman avatar Oct 19 '23 22:10 tazimmerman

@dlsniper, what kind of testing do you have in mind? I have used it with sqlserver alot, but I don't know how much work it would be to add it to the test environment.

kmpm avatar Feb 01 '24 16:02 kmpm

Hi @kmpm I've tagged this as needs testing because it's something that would benefit having the automated CI tests in place before merging. I plan to integrate that in the extremely near future and then I think it will be good to go. I'll take a closer look at it in the next couple of weeks or so, there are plenty PRs to get to and this is more complex than some of the other ones.

Thank you for being so patient about it and for your contribution.

dlsniper avatar Feb 01 '24 19:02 dlsniper

The most complex thing with this PR is actually the test. The funtional part of the code is actually quite straight forward .

kmpm avatar Feb 01 '24 19:02 kmpm

Hi @dsniper, what kind of tests are you missing. Preferably there should be some tests for mssql but that is not one of the database engines in the default test environment for this project.

kmpm avatar May 24 '24 08:05 kmpm

I recently ran into this problem (and I'm using MSSQL) and it'd be nice if NextResultSet invalidated the cache. sqlx lacks testing for MSSQL across the board so holding up this PR for MSSQL tests doesn't make much sense and would be nice to add those in a subsequent PR.

tankbusta avatar Aug 27 '24 17:08 tankbusta

I have been dabbling with adding mssql into sqlx_test.go but I keep having issues with general database syntax differences like.

  1. mssql uses SELECT TOP(1)... instead of ending with ... LIMIT 1
  2. mssql cant compare a column with type text with string that's get cast into mssql nvarchar
  3. mssql does not have natural join
  4. probably more

Changes to the test framework would be far bigger than the changes to implement NextResultSet. It's not optimal but I do not have time or the "urge" to rewrite everything necessary to add mssql tests, I would rather not be using sqlx since that would actually save time in comparison to change the test framework.

If creating some non generic tests just for this that uses mssql I would like to have some directions on how you would like to have that done.

kmpm avatar Aug 27 '24 20:08 kmpm

Is there anything that we are waiting for to merge this change? Changes to the test framework could be done as separated issue/task.

ofen avatar Sep 05 '24 10:09 ofen

As for me this PR is ready to be merged. I'll gladly write some more tests later when and if the testing framework support mssql.

kmpm avatar Sep 17 '24 08:09 kmpm