Skip to content

docs: fix kl_divergence and chi_square audit arguments and semantics - #6128

Open
shinichitazawa wants to merge 1 commit into
SQLMesh:mainfrom
shinichitazawa:docs/fix-kl-divergence-chi-square-args
Open

shinichitazawa wants to merge 1 commit into
SQLMesh:mainfrom
shinichitazawa:docs/fix-kl-divergence-chi-square-args

Conversation

@shinichitazawa

Copy link
Copy Markdown

Description

The examples for two statistical audits in docs/concepts/audits.md use argument names that the built-in audits do not accept. Copying them fails at render time with Macro variable 'source_column' is undefined.

Audit Documented Implemented (sqlmesh/core/audit/builtin.py)
kl_divergence column, target_column source_column, target_column
chi_square column, target_column column_a, column_b (plus dependent)

The chi_square description also said the audit "ensures that the chi-square statistic ... does not exceed a critical value". The implementation does the opposite by default:

HAVING NOT @IF(@dependent, chi_square > @critical_value, chi_square <= @critical_value)

With dependent := true (the default) the audit asserts that the two columns are dependent and fails when the statistic does not exceed the critical value. This PR documents the dependent argument and both behaviours, updates the example text accordingly, and fixes the "statistic0" typo. It also aligns the kl_divergence usage comment in builtin.py with its arguments.

Note: even with the corrected argument names, kl_divergence currently fails at run time on DuckDB 1.5.6 and Trino 483 (LEFT JOIN ... USING (@source_column) requires both columns to share a name, and when they do, num_rows becomes ambiguous). That is an implementation issue outside the scope of this docs change.

Test Plan

Ran each audit with SQLMesh 0.236.3 on DuckDB 1.5.6 against a single hourly interval (INCREMENTAL_BY_TIME_RANGE model, ~329k rows):

  • chi_square(column := ..., target_column := ...) (current docs): fails with Macro variable ... is undefined
  • chi_square(column_a := src_ns, column_b := dst_ns, critical_value := 6.635): passes (statistic ~889,337, columns are dependent)
  • Same with dependent := false: fails with 1 row failed, confirming the documented semantics
  • kl_divergence(column := ...) (current docs): fails with Macro variable 'source_column' is undefined; with source_column := the macro resolves (run-time failure described above)

ruff check / ruff format --check pass on builtin.py (comment-only change). make fast-test was not run since no code paths changed.

Checklist

  • I have run make style and fixed any issues (ran ruff check and ruff format --check on the changed Python file; docs are not covered by pre-commit hooks)
  • I have added tests for my changes (if applicable) — not applicable, docs and a comment only
  • All existing tests pass (make fast-test) — not run, no code paths changed
  • My commits are signed off (git commit -s) per the DCO

🤖 Generated with Claude Code

The documented examples used argument names that the built-in audits do not
accept, so copying them fails with "Macro variable 'source_column' is undefined":

- kl_divergence takes source_column / target_column (not column / target_column)
- chi_square takes column_a / column_b (not column / target_column)

The chi_square description also said the audit ensures the statistic does not
exceed the critical value. The implementation does the opposite by default:
with dependent := true (the default) it asserts that the columns are dependent
and fails when the statistic does not exceed the critical value. Document the
dependent argument and both behaviours, and fix the "statistic0" typo.

Also align the kl_divergence usage comment in builtin.py with its arguments.

Signed-off-by: shinichitazawa <shinichitazawa.aws@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant