Repository navigation
Conversation
…_http_method The z_score audit only applied `condition` to the CTE that computes the mean and standard deviation. The outer query scanned every row of the model, so rows excluded by the condition were still compared against statistics computed from a different subset and reported as failures. The valid_http_method audit had no `condition` default and did not reference it in its query, so a `condition` passed to it was silently ignored, unlike every other built-in audit. Apply `condition` in the WHERE clause of both queries. Signed-off-by: Dhruv Gupta <todhruvg@gmail.com>
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
Fixes #6155
Two built-in audits did not apply the optional
conditionargument to the rows they check:z_scoreappliedconditiononly in thestatsCTE that computes the mean and standard deviation. The outer query read every row of the model, so rows excluded by the condition were compared against statistics from a different subset and reported as failures. It also scanned the full model instead of the filtered rows.valid_http_methodhad noconditiondefault and did not reference@condition, so aconditionpassed to it was silently ignored. Every other built-in audit supports it.This change adds
@conditionto the outerWHEREclause ofz_score. It also givesvalid_http_methodthe sameconditiondefault (TRUE) and@AND(..., @condition)filter that the other built-in audits use. Without acondition, both queries return the same rows as before. The_non_blockingvariants are copies of these audits, so they get the fix too.Test Plan
test_z_score_auditandtest_valid_http_method_auditintests/core/test_audit.py. They check the rendered SQL with and without acondition.test_builtin_audits_condition_filters_audited_rows, which runs both audits against an in-memory DuckDB table where the condition excludes some rows. Before this change,z_scoreflagged the excluded rows andvalid_http_methodflagged rows outside the condition.mainand pass with this change.pytest tests/core/test_audit.py: 46 passed.pytest -n 8 -m "fast and not cicdonly and not isolated" tests/core: 2105 passed, 2 skipped. 6 failures intests/core/test_connection_config.py(mssql/fabric pyodbc) come frompyodbcnot being installed locally, and they fail the same way onmain.SKIP=prettier,eslint pre-commit run --all-files: ruff and ruff-format pass. mypy passes on the changed files; across the tree it only reportsimport-not-foundfor optional extras (web, lsp) that are not installed in my local environment.Checklist
make styleand fixed any issuesmake fast-test) (ran thetests/corefast subset locally, see above)git commit -s) per the DCO