Repository navigation
docs: fix kl_divergence and chi_square audit arguments and semantics - #6128
Open
shinichitazawa wants to merge 1 commit into
Open
shinichitazawa wants to merge 1 commit into
shinichitazawa wants to merge 1 commit into
Conversation
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>
2 of 4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The examples for two statistical audits in
docs/concepts/audits.mduse argument names that the built-in audits do not accept. Copying them fails at render time withMacro variable 'source_column' is undefined.sqlmesh/core/audit/builtin.py)kl_divergencecolumn,target_columnsource_column,target_columnchi_squarecolumn,target_columncolumn_a,column_b(plusdependent)The
chi_squaredescription also said the audit "ensures that the chi-square statistic ... does not exceed a critical value". The implementation does the opposite by default: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 thedependentargument and both behaviours, updates the example text accordingly, and fixes the "statistic0" typo. It also aligns thekl_divergenceusage comment inbuiltin.pywith its arguments.Note: even with the corrected argument names,
kl_divergencecurrently 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_rowsbecomes 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_RANGEmodel, ~329k rows):chi_square(column := ..., target_column := ...)(current docs): fails withMacro variable ... is undefinedchi_square(column_a := src_ns, column_b := dst_ns, critical_value := 6.635): passes (statistic ~889,337, columns are dependent)dependent := false: fails with1 row failed, confirming the documented semanticskl_divergence(column := ...)(current docs): fails withMacro variable 'source_column' is undefined; withsource_column :=the macro resolves (run-time failure described above)ruff check/ruff format --checkpass onbuiltin.py(comment-only change).make fast-testwas not run since no code paths changed.Checklist
make styleand fixed any issues (ranruff checkandruff format --checkon the changed Python file; docs are not covered by pre-commit hooks)make fast-test) — not run, no code paths changedgit commit -s) per the DCO🤖 Generated with Claude Code