analyze_column(): handle null values in the common-value analysis

sqlite_utils/db.py

@@ -5323,13 +5323,22 @@ def analyze_column(
             total_rows = db[table].count
 
         def truncate(value):
+            if value is None:
+                return None
             if value_truncate is None or isinstance(value, (float, int)):
                 return value
             value = str(value)
             if len(value) > value_truncate:
                 value = value[:value_truncate] + "..."
             return value
 
+        def sort_key(pair):
+            # Counts are the primary key. Ties are broken on the value
+            # itself, so keep nulls out of the comparison - None cannot be
+            # ordered against a string or a number - and sort them last.
+            value = pair[0]
+            return (pair[1], value is not None, "" if value is None else value)
+
         table_quoted = quote_identifier(table)
         column_quoted = quote_identifier(column)
         num_null = db.execute(
@@ -5343,7 +5352,8 @@ def truncate(value):
         ).fetchone()[0]
         most_common_results = None
         least_common_results = None
-        if num_distinct == 1:
+        if num_distinct == 1 and not num_null:
+            # Every row holds that one value, so skip the group by
             value = db.execute(
                 f"select {column_quoted} from {table_quoted} limit 1"
             ).fetchone()[0]
@@ -5363,7 +5373,7 @@ def truncate(value):
                             f"limit {common_limit}"
                         ).fetchall()
                     ]
-                    most_common_results.sort(key=lambda p: (p[1], p[0]), reverse=True)
+                    most_common_results.sort(key=sort_key, reverse=True)
             if least_common:
                 if num_distinct <= common_limit:
                     # No need to run the query if it will just return the results in reverse order
@@ -5378,7 +5388,7 @@ def truncate(value):
                             f"limit {common_limit}"
                         ).fetchall()
                     ]
-                    least_common_results.sort(key=lambda p: (p[1], p[0]))
+                    least_common_results.sort(key=sort_key)
         return ColumnDetails(
             self.name,
             column,

tests/test_analyze.py

@@ -51,3 +51,67 @@ def test_analyze_index_by_name(db):
     assert list(db.table("sqlite_stat1").rows) == [
         {"tbl": "two_indexes", "idx": "idx_two_indexes_species", "stat": "1 1"},
     ]
+
+
+@pytest.mark.parametrize(
+    "values,expected_num_distinct",
+    (
+        # count(distinct col) skips nulls, which is why a column of nulls
+        # reports no distinct values at all
+        ([None, None, None], 0),
+        ([None, None, "a", "a"], 1),
+        (["a", "a", "b", "b"], 2),
+    ),
+)
+def test_analyze_column_num_distinct_ignores_nulls(
+    fresh_db, values, expected_num_distinct
+):
+    fresh_db["t"].insert_all([{"c": value} for value in values])
+    assert fresh_db["t"].analyze_column("c").num_distinct == expected_num_distinct
+
+
+@pytest.mark.parametrize("value_truncate", (None, 80))
+def test_analyze_column_most_common_includes_nulls(fresh_db, value_truncate):
+    # One distinct non-null value plus nulls used to take the
+    # "single distinct value" shortcut, which reported that one value
+    # with a count of total_rows and left the nulls out entirely
+    fresh_db["t"].insert_all([{"c": None}, {"c": None}, {"c": "a"}, {"c": "a"}])
+    details = fresh_db["t"].analyze_column(
+        "c", total_rows=4, value_truncate=value_truncate
+    )
+    assert dict(details.most_common) == {None: 2, "a": 2}
+
+
+@pytest.mark.parametrize("value_truncate", (None, 80))
+def test_analyze_column_most_common_ties_with_nulls(fresh_db, value_truncate):
+    # A null and a string with the same count used to be compared
+    # directly by the tie-break sort, which raised TypeError
+    fresh_db["t"].insert_all(
+        [{"c": None}, {"c": None}, {"c": "a"}, {"c": "a"}, {"c": "b"}]
+    )
+    details = fresh_db["t"].analyze_column("c", value_truncate=value_truncate)
+    assert dict(details.most_common) == {None: 2, "a": 2, "b": 1}
+
+
+def test_analyze_column_least_common_ties_with_nulls(fresh_db):
+    # A null and a string share the lowest count, so the tie-break has
+    # to order them without comparing None against a string
+    fresh_db["t"].insert_all(
+        [{"c": None}, {"c": "a"}] + [{"c": c} for c in "bcdefg" for _ in range(2)]
+    )
+    details = fresh_db["t"].analyze_column("c", common_limit=5)
+    assert dict(details.least_common) == {
+        None: 1,
+        "a": 1,
+        "e": 2,
+        "f": 2,
+        "g": 2,
+    }
+
+
+def test_analyze_column_does_not_render_null_as_the_string_none(fresh_db):
+    # analyze-tables passes value_truncate=80, and the str() of a null
+    # was indistinguishable from a column that really holds "None"
+    fresh_db["t"].insert_all([{"c": None}, {"c": None}, {"c": "None"}])
+    details = fresh_db["t"].analyze_column("c", total_rows=3, value_truncate=80)
+    assert dict(details.most_common) == {None: 2, "None": 1}