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

Added missing connection parameter to the dbplyr_fill0 implementation #753

Merged
merged 4 commits into from
Feb 7, 2022
Merged

Conversation

rsund
Copy link
Contributor

@rsund rsund commented Jan 14, 2022

The connection parameter was not passed to the first translate_sql() call in the dbplyr_fill0 implementation for databases without support for IGNORE NULLS. Because of that the quotes other than ` ` were not correct for that particular translate_sql() call.

Following the example in the fill documentation the produced SQL was as follows with a database that uses " " as quotes:

<SQL>
SELECT "group", "name", "role", MAX("n_squirrels") OVER (PARTITION BY "..dbplyr_partion_1") AS "n_squirrels", 
MAX("n_squirrels2") OVER (PARTITION BY "..dbplyr_partion_2") AS "n_squirrels2", "id" FROM
(SELECT "group", "name", "role", "n_squirrels", "n_squirrels2", "id", SUM(CASE WHEN (((`n_squirrels`) IS NULL)) THEN 0 
ELSE 1 END) OVER (ORDER BY "id" ROWS UNBOUNDED PRECEDING) AS "..dbplyr_partion_1", 
SUM(CASE WHEN (((`n_squirrels2`) IS NULL))  THEN 0 ELSE 1 END) OVER (ORDER BY "id" ROWS UNBOUNDED PRECEDING) AS
"..dbplyr_partion_2" FROM "squirrels") "q01"

So following the SUM(CASE WHEN... there are wrong quotes in two places (around the variable names).

The fix was simple and only requires that the missing connection parameter is added to the translate_sql() call as shown in the commit.

@rsund
Copy link
Contributor Author

rsund commented Jan 14, 2022

Interestingly the tests failed. I should have been testing more carefully before sending a PR, but I couldn't anticipate that such a simple change had such consequences, especially as things were working just fine with the database with " " quotes after the commit.

Any idea what should be changed?

Maybe instead of adding a connection parameter, the current connection could be stored with the function set_current_con() in the beginning of the dbplyr_fill0() function so that the engine with " " quotes could fetch the connection (in the translate_sql() there is a line for fetching the con if parameter is missing: con <- con %||% sql_current_con() %||% simulate_dbi()).

But I still don't quite get that why adding a clearly missing (and important) parameter changes the behavior in CASE WHEN translation?!

@mgirlich
Copy link
Collaborator

Good catch, thanks for the PR. The tests fail due to an incorrect check when translating case_when() that only happens for SQLite. I created a PR to fix this. Once this is merged, the tests pass.

It would be great if you could also add a NEWS entry.

@rsund
Copy link
Contributor Author

rsund commented Jan 14, 2022

OK, good to hear. A commit for the NEWS entry added as requested.

@mgirlich mgirlich merged commit b34e8a3 into tidyverse:main Feb 7, 2022
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