Repository: sqlfluff/sqlfluff

Issue:
Rule: consistent syntax for COUNT(*) / COUNT(1) -> COUNT ALL
I came across this post on LinkedIn: https://www.linkedin.com/posts/eczachly_dataengineering-activity-6775447325725769728-EhK3 advocating to always use `COUNT(1)` vs `COUNT(*)`.

My personal opinion, not that it matters in the context of implementation of the rule, is that both syntaxes are a way to say "COUNT all rows in this query". In many (most?) cases it seems like the engine optimizers just make these queries equivalent.

There are many "urban myths", for example:

1. `COUNT(*)` is worse because `*` is actually pulling the full row (which would be painful for columnar databases)

But in this [Stack Overflow answer](https://stackoverflow.com/a/1221649) you see the [ANSI-92 spec](http://msdn.microsoft.com/en-us/library/ms175997.aspx) quoted `If COUNT(*) is specified, then the result is the cardinality of T.`

2. `COUNT(1)` is worse because it evaluates the expression "1" against all of the rows in the table

In the same Stack Overflow answer: `COUNT(1) has been optimized out by RDBMS vendors because of this superstition.`

Now, the only way to know **know** would be to see the plan that each database gives you for these queries, or/and actually compare performance, since most of these engines are closed source.

I actually had a look into the Spark code, and there was [this interesting comment](https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala#L1795):

> [...] while count(*) will be converted to count(1)

So my conclusion from this all:
1. An issue that people have strong opinions about.
2. Something that does not seem to actually have an impact on performance.
3. A great candidate for a rule.

I would suggest a simple rule that enforces consistency of either `COUNT(*)` or `COUNT(1)` across the code base (configurable), and `COUNT(*)` by default since it is specifically mentioned in the ANSI docs.

 

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/sqlfluff/sqlfluff/pull/860?src=pr&el=h1) Report
> Merging [#860](https://codecov.io/gh/sqlfluff/sqlfluff/pull/860?src=pr&el=desc) (72de328) into [master](https://codecov.io/gh/sqlfluff/sqlfluff/commit/c579ca3ec7c0a83a04e40aa94fe9478486198b04?el=desc) (c579ca3) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/sqlfluff/sqlfluff/pull/860/graphs/tree.svg?width=650&height=150&src=pr&token=maM8kLgCmn)](https://codecov.io/gh/sqlfluff/sqlfluff/pull/860?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master     #860   +/-   ##
=======================================
  Coverage   96.22%   96.23%           
=======================================
  Files         107      108    +1     
  Lines        7150     7162   +12     
=======================================
+ Hits         6880     6892   +12     
  Misses        270      270           
```

| Flag | Coverage Δ | |
|---|---|---|
| dbt017-py38 | `72.19% <83.33%> (+0.01%)` | :arrow_up: |
| dbt018-py38 | `72.22% <83.33%> (+0.01%)` | :arrow_up: |
| dbt019-py38 | `72.22% <83.33%> (+0.01%)` | :arrow_up: |
| py36 | `95.42% <100.00%> (+<0.01%)` | :arrow_up: |
| py37 | `95.42% <100.00%> (+<0.01%)` | :arrow_up: |
| py38 | `95.38% <100.00%> (+<0.01%)` | :arrow_up: |
| py39 | `95.38% <100.00%> (+<0.01%)` | :arrow_up: |
| win-dbt018-py37 | `?` | |
| win-py37 | `?` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/sqlfluff/sqlfluff/pull/860?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/sqlfluff/core/rules/config\_info.py](https://codecov.io/gh/sqlfluff/sqlfluff/pull/860/diff?src=pr&el=tree#diff-c3JjL3NxbGZsdWZmL2NvcmUvcnVsZXMvY29uZmlnX2luZm8ucHk=) | `100.00% <ø> (ø)` | |
| [src/sqlfluff/core/rules/std/L044.py](https://codecov.io/gh/sqlfluff/sqlfluff/pull/860/diff?src=pr&el=tree#diff-c3JjL3NxbGZsdWZmL2NvcmUvcnVsZXMvc3RkL0wwNDQucHk=) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/sqlfluff/sqlfluff/pull/860?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/sqlfluff/sqlfluff/pull/860?src=pr&el=footer). Last update [c579ca3...72de328](https://codecov.io/gh/sqlfluff/sqlfluff/pull/860?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

barrywhart: I think I have a PR still open from a few weeks ago that creates rule numbers 44 and 45. Would you be okay renumbering this rule?

I've gotten a few comments on that PR, but no full reviews yet. 😢

Patch:
diff --git a/src/sqlfluff/core/default_config.cfg b/src/sqlfluff/core/default_config.cfg
index b83f9195f..da51286c6 100644
--- a/src/sqlfluff/core/default_config.cfg
+++ b/src/sqlfluff/core/default_config.cfg
@@ -70,3 +70,6 @@ capitalisation_policy = consistent
 [sqlfluff:rules:L042]
 # By default, allow subqueries in from clauses, but not join clauses.
 forbid_subquery_in = join
+
+[sqlfluff:rules:L047]  # Consistent syntax to count all rows
+prefer_count_1 = False
diff --git a/src/sqlfluff/core/rules/config_info.py b/src/sqlfluff/core/rules/config_info.py
index 701c5516e..697f308b8 100644
--- a/src/sqlfluff/core/rules/config_info.py
+++ b/src/sqlfluff/core/rules/config_info.py
@@ -80,6 +80,10 @@ STANDARD_CONFIG_INFO_DICT = {
         "validation": ["join", "from", "both"],
         "definition": "Which clauses should be linted for subqueries",
     },
+    "prefer_count_1": {
+        "validation": [True, False],
+        "definition": ("Should count(1) be preferred over count(*)?"),
+    },
 }
 
 
diff --git a/src/sqlfluff/core/rules/std/L047.py b/src/sqlfluff/core/rules/std/L047.py
new file mode 100644
index 000000000..4bedd1e17
--- /dev/null
+++ b/src/sqlfluff/core/rules/std/L047.py
@@ -0,0 +1,105 @@
+"""Implementation of Rule L047."""
+
+from sqlfluff.core.rules.base import BaseRule, LintFix, LintResult
+from sqlfluff.core.rules.doc_decorators import (
+    document_configuration,
+    document_fix_compatible,
+)
+
+
+@document_configuration
+@document_fix_compatible
+class Rule_L047(BaseRule):
+    """Use consistent syntax to express "count number of rows".
+
+    COUNT(*) and COUNT(1) are equivalent syntaxes in many SQL engines
+    due to optimizers interpreting these instructions as
+    "count number of rows in result".
+
+    The ANSI-92_ spec mentions the COUNT(*) syntax specifically as
+    having a special meaning:
+
+        If COUNT(*) is specified, then
+        the result is the cardinality of T.
+
+    So by default, SQLFluff enforces the consistent use of COUNT(*).
+
+    If the SQL engine you work with, or your team, prefers COUNT(1)
+    over COUNT(*) you can configure this rule to consistently enforce COUNT(1).
+
+    .. _ANSI-92: http://msdn.microsoft.com/en-us/library/ms175997.aspx
+
+    | **Anti-pattern**
+
+    .. code-block:: sql
+
+        select
+            count(1)
+        from table_a
+
+    | **Best practice**
+    |   Use count(*) unless specified otherwise by config ``prefer_count_1``
+
+    .. code-block:: sql
+
+        select
+            count(*)
+        from table_a
+
+    """
+
+    config_keywords = ["prefer_count_1"]
+
+    def _eval(self, segment, **kwargs):
+        """Find rule violations and provide fixes."""
+        if (
+            segment.is_type("function")
+            and segment.get_child("function_name").raw_upper == "COUNT"
+        ):
+            f_content = [
+                seg
+                for seg in segment.segments
+                if not seg.is_meta
+                and not seg.is_type(
+                    "start_bracket",
+                    "end_bracket",
+                    "function_name",
+                    "whitespace",
+                    "newline",
+                )
+            ]
+            if len(f_content) != 1:
+                return None
+
+            if self.prefer_count_1 and f_content[0].type == "star":
+                return LintResult(
+                    anchor=segment,
+                    fixes=[
+                        LintFix(
+                            "edit",
+                            f_content[0],
+                            f_content[0].edit(f_content[0].raw.replace("*", "1")),
+                        ),
+                    ],
+                )
+            if not self.prefer_count_1 and f_content[0].type == "expression":
+                expression_content = [
+                    seg for seg in f_content[0].segments if not seg.is_meta
+                ]
+                if (
+                    len(expression_content) == 1
+                    and expression_content[0].type == "literal"
+                    and expression_content[0].raw == "1"
+                ):
+                    return LintResult(
+                        anchor=segment,
+                        fixes=[
+                            LintFix(
+                                "edit",
+                                expression_content[0],
+                                expression_content[0].edit(
+                                    expression_content[0].raw.replace("1", "*")
+                                ),
+                            ),
+                        ],
+                    )


Test patch:
diff --git a/test/fixtures/rules/std_rule_cases/L047.yml b/test/fixtures/rules/std_rule_cases/L047.yml
new file mode 100644
index 000000000..7f0810c70
--- /dev/null
+++ b/test/fixtures/rules/std_rule_cases/L047.yml
@@ -0,0 +1,118 @@
+rule: L047
+
+passes_on_count_star:
+  pass_str: |
+    select
+        foo,
+        count(*)
+    from my_table
+    group by
+      foo
+
+passes_on_count_1:
+  pass_str: |
+    select
+        foo,
+        count(1)
+    from my_table
+    group by
+      foo
+
+  configs: &prefer_count_1
+    rules:
+      L047:
+        prefer_count_1: True
+
+changes_count_1_to_count_star:
+  fail_str: |
+    select
+        foo,
+        count(1)
+    from my_table
+    group by
+      foo
+
+  fix_str: |
+    select
+        foo,
+        count(*)
+    from my_table
+    group by
+      foo
+
+handles_whitespaces:
+  fail_str: |
+    select
+        foo,
+        count( 1 )
+    from my_table
+    group by
+      foo
+
+  fix_str: |
+    select
+        foo,
+        count( * )
+    from my_table
+    group by
+      foo
+
+changes_count_star_to_count_1:
+  fail_str: |
+    select
+        foo,
+        count(*)
+    from my_table
+    group by
+      foo
+
+  fix_str: |
+    select
+        foo,
+        count(1)
+    from my_table
+    group by
+      foo
+
+  configs: *prefer_count_1
+
+changes_count_star_to_count_1_handle_new_line:
+  fail_str: |
+    select
+        foo,
+        count(
+
+          *
+
+        )
+    from my_table
+    group by
+      foo
+
+  fix_str: |
+    select
+        foo,
+        count(
+
+          1
+
+        )
+    from my_table
+    group by
+      foo
+
+  configs: *prefer_count_1
+
+no_false_positive_on_count_col:
+  pass_str: |
+    select
+        foo,
+        count(bar)
+    from my_table
+
+no_false_positive_on_expression:
+  pass_str: |
+    select
+        foo,
+        count(1 + 10)
+    from my_table