Skip to content

analyze_column() mis-handles columns containing nulls #887

Description

@feiiiiii5

Table.analyze_column() gives a wrong answer for columns containing nulls, in three separate ways. All three are in sqlite_utils/db.py, and all three come from nulls not being accounted for.

1. The "single distinct value" shortcut reports a count that is too high

count(distinct col) skips nulls in SQL. So a column holding nulls plus one other value reports num_distinct == 1 and takes the shortcut that skips the group by:

if num_distinct == 1:
    value = db.execute(f"select {column_quoted} from {table_quoted} limit 1").fetchone()[0]
    most_common_results = [(truncate(value), total_rows)]
>>> db["t"].insert_all([{"c": None}, {"c": None}, {"c": "a"}, {"c": "a"}])
>>> db["t"].analyze_column("c").most_common
[(None, 4)]
>>> db.execute("select c, count(*) from t group by c").fetchall()
[(None, 2), ('a', 2)]

One value, counted four times, with the two nulls not mentioned at all. The shortcut is only valid when every row holds that one value.

This is reachable from the CLI too — sqlite-utils analyze-tables passes value_truncate=80, and truncate() then renders the null as the string "None", so the output reads 4: None for a column that is half null.

2. A null tied on count with a string raises TypeError

Both sorts break ties on the value itself, and None cannot be ordered against a string:

>>> db["t"].insert_all([{"c": None}, {"c": "a"}] + [{"c": c} for c in "bcdefg" for _ in range(2)])
>>> db["t"].analyze_column("c", common_limit=5).least_common
TypeError: '<' not supported between instances of 'NoneType' and 'str'

most_common has the same shape. Note that problem 1 currently hides this for some inputs, since the shortcut skips the sort — so fixing the shortcut alone would turn a wrong answer into a crash for some of them.

3. A null renders as the string "None"

>>> db["t"].insert_all([{"c": None}, {"c": None}, {"c": "None"}])
>>> db["t"].analyze_column("c", total_rows=3, value_truncate=80).most_common
[('None', 2), ('None', 1)]

Two entries, both spelled "None", and the reader cannot tell which was a null. With --save that string is persisted into _analyze_tables_.most_common too.

A question I did not want to answer on my own

num_distinct itself follows SQL, so an all-null column reports 0 — test_analyze_table_column_all_nulls pins that. But that leaves num_distinct able to disagree with the list it accompanies:

>>> db["t"].insert_all([{"c": None}, {"c": "a"}, {"c": "b"}])
>>> d = db["t"].analyze_column("c")
>>> d.num_distinct, len(d.most_common)
(2, 3)

Counting null as a distinct value fixes that and is a one-line change, but it changes what Distinct values: means for every column containing nulls, and whether a null should count is a question about the number's meaning rather than a defect. I would rather you decided that.

What I propose

Three changes, all confined to analyze_column():

  • require no nulls for the single-distinct-value shortcut, so those columns fall through to the group by
  • a sort_key() that keeps nulls out of the tie-break comparison and sorts them last, preserving the current ordering of non-null ties (test_analyze_tables.py asserts "Terryterryterry" before "Kumar" at equal counts)
  • truncate() returns None unchanged instead of str()-ing it

Plus six cases in tests/test_analyze.py, all failing on 6bc1d33, and one that pins the current num_distinct semantics so the question above is documented either way.

Full suite and all six CI gates (mypy, pyright, flake8, ty, black --check, cog --check) pass with those changes applied.

I have this ready to send as a PR — say the word and I will open it, or if you would rather reshape it first, especially on the num_distinct question, tell me which way you want it and I will rework before sending anything.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions