diff --git a/sqlite_utils/db.py b/sqlite_utils/db.py index c011d9bb0..0c5d427ea 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,9 +5373,13 @@ 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: + # GROUP BY includes a null group, unlike count(distinct). + if ( + most_common_results is not None + and num_distinct + bool(num_null) <= common_limit + ): # No need to run the query if it will just return the results in reverse order least_common_results = None else: @@ -5378,7 +5392,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..410f14ff7 100644 --- a/tests/test_analyze.py +++ b/tests/test_analyze.py @@ -51,3 +51,89 @@ 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, + } + + +@pytest.mark.parametrize( + "values,common_limit,most_common,expected_most,expected_least", + ( + (["a", "a", "a", None], 1, True, [("a", 3)], [(None, 1)]), + ([None, None, None, "a"], 1, True, [(None, 3)], [("a", 1)]), + (["a", "a", "a", "b"], 2, True, [("a", 3), ("b", 1)], None), + (["a", "a", "a", None], 2, True, [("a", 3), (None, 1)], None), + (["a", "a", "a", None], 2, False, None, [(None, 1), ("a", 3)]), + (["a", "a", "a", None], 0, True, [], []), + ), +) +def test_analyze_column_only_skips_least_common_when_all_groups_are_returned( + fresh_db, values, common_limit, most_common, expected_most, expected_least +): + fresh_db["t"].insert_all([{"c": value} for value in values]) + details = fresh_db["t"].analyze_column( + "c", common_limit=common_limit, most_common=most_common + ) + assert details.most_common == expected_most + assert details.least_common == expected_least + + +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}