From 8e5dfdd40958e7475baf0cb543637f85671d52ff Mon Sep 17 00:00:00 2001 From: feiiiiii5 Date: Mon, 28 Sep 2026 18:55:53 +0800 Subject: [PATCH] analyze_column(): handle null values in the common-value analysis Three ways a column containing nulls produced a wrong answer, all from nulls not being accounted for. 1. The "single distinct value" shortcut. count(distinct col) skips nulls in SQL, so a column holding nulls plus one other value reports num_distinct == 1 and took the shortcut, which reports that one value with a count of total_rows: insert [{'c': None}, {'c': None}, {'c': 'a'}, {'c': 'a'}] analyze_column('c').most_common # was [(None, 4)] select c, count(*) group by c # is [(None, 2), ('a', 2)] The shortcut now requires that the column has no nulls, so those columns fall through to the group by like any other. 2. The tie-break sort compared the values directly, so a null and a string with the same count raised TypeError: analyze_column('c', common_limit=5) # TypeError: '<' not supported between instances of 'NoneType' and 'str' sort_key() keeps nulls out of the comparison and sorts them last, which keeps the existing ordering of non-null ties - the test for 'Terryterryterry' ahead of 'Kumar' at equal counts still passes. 3. truncate() turned a null into the string 'None', so a null was indistinguishable from a column really holding 'None'. That is the path analyze-tables takes, since it passes value_truncate=80. num_distinct itself is unchanged: it still follows SQL and reports 0 for an all-null column, which test_analyze_table_column_all_nulls pins. But num_distinct can therefore be lower than len(most_common) when a column holds nulls alongside all-distinct values - {'c': None, 'a', 'b'} gives num_distinct 2 and three most_common entries. Whether null should count as a distinct value is a question about what the number means, so I have left it alone rather than change an output the tests already pin. Tests: 6 new cases in tests/test_analyze.py, all failing on 6bc1d33. Suite 1506 passed, 16 skipped. mypy, pyright, flake8, ty, black and cog --check clean. --- sqlite_utils/db.py | 16 +++++++++-- tests/test_analyze.py | 64 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 3 deletions(-) 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}