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>
|
@cmgoffena13 @mday-io would one of you be able to review this one? It fixes #6155: the |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Apply conditions to audit failures sqlmesh/core/audit/builtin.py |
The HTTP-method audit now includes condition in its failure predicate. The z-score audit applies condition to the rows checked for threshold violations. |
Verify condition filtering tests/core/test_audit.py |
SQL-rendering tests cover both audits with and without conditions. A DuckDB test checks their conditioned results. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Merge Risk: 
fafe4
-
The conditioned audits restrict reported failures as intended. No actionable merge-blocking issue was identified.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely identifies the primary change: applying the condition to rows checked by the z_score and valid_http_method audits. Description check The description includes the required Description, Test Plan, and Checklist sections. It explains the behavior change, implementation, tests, results, and known local test limitations. The unchecked f… Linked Issues check Issue #6155requiresz_scoreto applyconditionwhen computing statistics and when selecting rows, and requiresvalid_http_methodto accept and applycondition. The reviewed diff adds `@condit…Out of Scope Changes check The changed production code is limited to the two audits named in issue #6155. The added tests verify their rendered SQL and condition-filtered results. These changes directly support the issue object…Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Full details: Description check
-
Explanation
The description includes the required Description, Test Plan, and Checklist sections. It explains the behavior change, implementation, tests, results, and known local test limitations. The unchecked full-test item is explicitly addressed with the fast-subset results and pyodbc limitation.
Full details: Linked Issues check
-
Explanation
Issue
#6155requiresz_scoreto applyconditionwhen computing statistics and when selecting rows, and requiresvalid_http_methodto accept and applycondition. The reviewed diff adds@conditionto the outerz_scorefilter. It adds theTRUEdefault and@AND(..., @condition)filter tovalid_http_method. The supplied change summary reports rendered-SQL tests and a DuckDB execution test for both audits. The PR description also states that the_non_blockingvariants are copies and receive these fixes.
Full details: Out of Scope Changes check
-
Explanation
The changed production code is limited to the two audits named in issue
#6155. The added tests verify their rendered SQL and condition-filtered results. These changes directly support the issue objectives. No unrelated change is shown.
Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 2 files.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
|
Simple patch. Should be an easy merged. Kicked off the checks and we'll see what happens |
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 DCOSummary by CodeRabbit