Remove method count.tbl_sql()

[mgirlich]

dplyr defines a method count.tbl_sql(). dbplyr defines a method count.tbl_lazy(). Now, the dplyr method wins due to the classes of database tables. I guess it would make sense to simply remove the method from dplyr (same issue for tally.tbl_sql()).

[DavisVaughan]

Supposedly this code should be running

dplyr/R/zzz.r

Lines 13 to 17 in 3a07390

	has_dbplyr <- is_installed("dbplyr")
	if (!has_dbplyr || !exists("count.tbl_sql", ns_env("dbplyr"))) {
	s3_register("dplyr::count", "tbl_sql")
	s3_register("dplyr::tally", "tbl_sql")
	}

[DavisVaughan]

Oh but its count.tbl_lazy in dbplyr? I guess that was a change? Or has it always been wrong?

[DavisVaughan]

It looks like count() was made generic here, which is also when the tbl_sql method was added:
#5633

Making count() generic seems to be a direct response from a request to do so so it can be used in dbplyr here
tidyverse/dbplyr@4dbcb44

The original dplyr PR was using tbl_sql, and the original dbplyr PR was using tbl_lazy, so I'm guessing it has been wrong this whole time? Romain even says in this comment that he didn't test it "with a dbplyr that has the methods" so he wouldn't have seen that it was broken #5633 (comment)

I think we could try to:

• Remove the methods
• Remove the onLoad conditional registration
• Remove the if (is.data.frame(x)) check from count.data.frame() and always unconditionally reconstruct. This seems to be a "hack" that was added to support data bases, added in Get count() working in database backends #5114, which wouldn't be needed if we removed these methods. It looks like that hack was added to add_count() then too, but has already been removed from there since then.
• Maybe add a test for count() returns group with >=2 groups in ... dbplyr#940 that is conditional on the presence of dbplyr to prove that their count() method is being used

[mgirlich]

That was fast 🏎️ Thanks a lot 😄