search

r-dbi / odbc

Connect to ODBC databases (using the DBI interface)
https://odbc.r-dbi.org/
Other
391 stars 107 forks source link

remove redundant row limit in table preview #768

Closed simonpcouch closed 8 months ago

simonpcouch commented 8 months ago

I'm not sure why it is that we pass rowLimit both to odbcPreviewQuery() and dbGetQuery()? Introduced in https://github.com/r-dbi/odbc/commit/111cedfe2aacb1ca4f87016814fb7d295f7dfb3f. From my understanding, odbcPreviewQuery() should limit the number of rows returned effectively and we can leave dbGetQuery(n) as its default value, -1, which will be passed to dbFetch() and interpreted as "return all available rows."

Closes #142, possibly; I wonder if Redshift trips up on a dbFetch(n) value greater than what the query actually returns. I don't have a Redshift account set up for myself but will ask the customer to test.

EDIT: if this does do the trick, we could split this change off into a Redshift method so that it doesn't apply to other DBMS.

hadley commented 8 months ago

The current code feels like a belt and bracers type approach and I don't think it should be harmful. Have we confirmed that #142 is still an issue? Looking at the issue now, I would have thought that the introduction of odbcPreviewQuery() would have already fixed it.

simonpcouch commented 8 months ago

Internal ticket 98494 presents quite similarly, though I agree that I thought odbcPreviewQuery() would have solved this. #142 may be unrelated.

simonpcouch commented 8 months ago

Closing in favor of #769.