diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index c011d9bb0..10783ede3 100644 --- a/sqlite_utils/db.py +++ b/sqlite_utils/db.py @@ -5323,6 +5323,8 @@ 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) @@ -5330,6 +5332,13 @@ def truncate(value): 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, diff --git a/tests/test_analyze.py b/tests/test_analyze.py index edd51745f..a930cbcb0 100644 --- a/tests/test_analyze.py +++ b/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}