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

Default way of translating n() to sql code should be count(*) and not count() #343

Closed
kondofersky opened this issue Aug 6, 2019 · 3 comments
Labels
bug an unexpected problem or unintended behavior func trans 🌍 Translation of individual functions to SQL
Milestone

Comments

@kondofersky
Copy link

Problem is described in the closed issue 314 .

Particularly obvious if one tries to use e.g. the corrr:::correlate() function, where obviously n() is needed to calculate the number of rows. The error message provided says that the count function requires 1 argument but 0 are provided. I tested the count() vs. count(*) on sqlfiddle and the version without * did not work for any of the possible examples, including versions of PostgreSQL, MySQL, Oracle, SQLite. I actually do not know a single sql language where this meight work (although I am not very experienced tbh).

Workaround is described by @hadley here but I think changing the translation to count(*) is the proper way of dealing with this.

@hadley
Copy link
Member

hadley commented Aug 6, 2019

My copy of "SQL-99 complete, really" states that there are two forms for COUNT(): COUNT(*) and COUNT(<column expression>). Not sure how I managed to create a default translation that doesn't work anywhere!

@kondofersky
Copy link
Author

Well, I am not sure if you are being sarcastic here but for what it’s worth, I really tried the count function without an argument in all of the provided languages and it did not work. I also asked more experienced SQL users (sample size = 3) but they all said that they always use count(*).

I would also provide a solution myself if you hint me where the translation of n() actually happens. Yesterday, I tried to dig deep and was at some point looking into the C code of eval_tidy from the rlang package. Not sure if this is the right path though...

@hadley
Copy link
Member

hadley commented Aug 6, 2019

The implementation is at https://github.com/tidyverse/dbplyr/blob/master/R/backend-.R#L292, but once you've fixed it in the base implementation, you'll also need to look through the other backends and remove their custom n() translations (since they'll no longer be needed).

@hadley hadley added bug an unexpected problem or unintended behavior func trans 🌍 Translation of individual functions to SQL labels Dec 13, 2019
@hadley hadley added this to the 2.0.0 milestone Dec 13, 2019
@hadley hadley closed this as completed in e746aed Sep 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug an unexpected problem or unintended behavior func trans 🌍 Translation of individual functions to SQL
Projects
None yet
Development

No branches or pull requests

2 participants