APP-1123: add skip_test_result_rows to skip test_result_rows query in get_test_results macro

[MikaKerman]

Summary

Adds a new skip_test_result_rows parameter to elementary_cli.get_test_results and threads it from ReportAPI.get_report_data through TestsAPI → TestsFetcher → the dbt macro. When skip_test_result_rows=true, the macro skips the get_result_rows_agate query against test_result_rows entirely, and the result_rows column on elementary_test_results is also omitted from the main query (no longer materialized into the intermediate ordered_test_results table on ClickHouse either).

Ticket: APP-1123

Why a new parameter instead of reusing disable_samples

ReportAPI.get_report_data already had a disable_samples parameter, but it has a different concern than what we needed:

• disable_samples (existing, Python-level) — applied by _get_test_result_from_test_result_db_row after the SQL has already run, and only sets sample_data = None for test_type == "dbt_test" rows. This is the OSS --disable-samples CLI behavior for PII protection: it masks dbt-test failed-row samples but preserves anomaly metrics and schema-change diffs.
• skip_test_result_rows (new, macro-level) — a pure DB optimization. Skips the expensive test_result_rows table query (~522 GB / 249.6M rows in cloud) and omits the result_rows column from the main elementary_test_results select. Affects all test types uniformly. The Python-level masking still applies on top of this for dbt-test rows when disable_samples is also set.

These are two separate concerns and the cloud consumer wants the optimization unconditionally (regardless of the per-env PII flag), so a new parameter cleanly separates them.

Changes

• elementary/monitor/dbt_project/macros/get_test_results.sql — add skip_test_result_rows = false to the dispatch macro, the default__ / fabric__ / clickhouse__ variants, and _process_raw_test_results. Wrap the get_result_rows_agate call in {% if not skip_test_result_rows %} (empty dict otherwise — _process_raw_test_results.test_result_rows_agate.get(test.id) returns None for missing keys). Also gate the get_test_rows_sample call inside _process_raw_test_results on the same flag so samples can't leak via the legacy elementary_test_results.result_rows JSON column.
• elementary/monitor/dbt_project/macros/base_queries/current_tests_run_results_query.sql — accept skip_test_result_rows and conditionally select null as result_rows instead of elementary_test_results.result_rows. This means the result_rows column is never materialized on Snowflake/BQ/Redshift/Postgres/Databricks and never inserted into ClickHouse's intermediate ordered_test_results table either.
• elementary/monitor/fetchers/tests/tests.py — accept skip_test_result_rows and forward it in macro_args.
• elementary/monitor/api/tests/tests.py — accept skip_test_result_rows in TestsAPI.__init__ and _get_test_results_db_rows, forward to the fetcher. The existing disable_samples parameter on get_test_results / _get_test_result_from_test_result_db_row is untouched.
• elementary/monitor/api/report/report.py — add new skip_test_result_rows: bool = False parameter, forward to the TestsAPI constructor. The existing disable_samples parameter and its forwarding to tests_api.get_test_results(disable_samples=...) is untouched.

Drive-by fix

• Initialize elementary_tests_allowlist_status in clickhouse__get_test_results. Pre-existing bug surfaced by CodeRabbit: the ClickHouse variant called _process_raw_test_results with an uninitialized variable, which would silently skip sample selection for non-dbt_test rows on ClickHouse. The default__ and fabric__ variants already set this from disable_passed_test_metrics.

Backwards compatibility

• All new parameters default to false, so existing callers (OSS users, data_monitoring_report.send_test_results_summary) see no behavior change.
• The existing disable_samples parameter on ReportAPI.get_report_data and TestsAPI.get_test_results is preserved with its original Python-level mask semantics — OSS edr report --disable-samples continues to work exactly as before.

Review & Testing Checklist for Human

• Verify the macro change covers all three dispatch variants (default__, fabric__, clickhouse__) by inspecting get_test_results.sql.
• Sanity-check that test_result_rows_agate = {} (empty dict) is safe input for _process_raw_test_results — it relies on test_result_rows_agate.get(test.id) which returns None for missing keys, and the get_test_rows_sample call is now also gated on skip_test_result_rows so it's never invoked when samples are skipped.
• Sanity-check the ClickHouse variant: with skip_test_result_rows=true, current_tests_run_results_query returns null for the result_rows column, so the inline INSERT INTO ordered_test_results no longer materializes it.
• Run an end-to-end report against a real warehouse with skip_test_result_rows=true and confirm the test_result_rows SELECT no longer appears in query history.

Notes

• This is the OSS side of the change. The cloud consumer (elementary-internal) bumps the pin to this commit and hardcodes skip_test_result_rows=True in get_edr_report while keeping disable_samples driven by the per-env disable_test_samples flag.

Link to Devin session: https://app.devin.ai/sessions/d3ae8f0bdf5b484b8ca0cde41ec58529
Requested by: @MikaKerman

Summary by CodeRabbit

• New Features
  • Added a new skip_test_result_rows option available across reporting, fetchers, and dbt macros.
  • When enabled, per-test result rows are omitted from retrievals and persisted result blobs are blanked.
  • Allows running without fetching or storing detailed per-test result rows when that detail isn’t required.

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment and CI monitoring

[github-actions]

👋 @MikaKerman
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Threads a new skip_test_result_rows flag through DBT macros, TestsFetcher, TestsAPI, and ReportAPI to allow callers to skip retrieval or persistence of per-test result rows.

Changes

skip_test_result_rows parameter implementation

Layer / File(s)	Summary
DBT macros: signatures and conditional result_rows elementary/monitor/dbt_project/macros/get_test_results.sql, elementary/monitor/dbt_project/macros/base_queries/current_tests_run_results_query.sql	Adds skip_test_result_rows to get_test_results, _process_raw_test_results, adapter-specific macros, and current_tests_run_results_query; conditionally omits/blank test_result_rows/result_rows when enabled.
TestsFetcher macro parameter forwarding elementary/monitor/fetchers/tests/tests.py	TestsFetcher.get_all_test_results_db_rows adds skip_test_result_rows and includes it in macro_args passed to the dbt macro.
TestsAPI parameter exposure and threading elementary/monitor/api/tests/tests.py	TestsAPI.__init__ and TestsAPI._get_test_results_db_rows add skip_test_result_rows and forward it to the fetcher.
ReportAPI skip_test_result_rows forwarding elementary/monitor/api/report/report.py	ReportAPI.get_report_data adds skip_test_result_rows and forwards it into the TestsAPI constructor.

Sequence Diagram

sequenceDiagram
  participant ReportAPI
  participant TestsAPI
  participant TestsFetcher
  participant DBT_macro as get_test_results
  ReportAPI->>TestsAPI: initialize(skip_test_result_rows)
  TestsAPI->>TestsFetcher: get_all_test_results_db_rows(skip_test_result_rows)
  TestsFetcher->>DBT_macro: run macro with macro_args(skip_test_result_rows)
  DBT_macro->>DBT_macro: conditional fetch/blank test_result_rows (skip when true)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I thread a tiny flag with care,
Through macros, fetchers, layers rare.
When rows are skipped and silence grows,
The pipeline hops where quiet flows.
Hooray — a tiny change, light as air.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the main change: adding a skip_test_result_rows parameter to the get_test_results macro to control test result row retrieval.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch devin/app-1123-disable-samples-macro-skip

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[linear]

APP-1123

[devin-ai-integration]

FYI: test (fabric) failed with a transient MSSQL connection error during dbt parse (before any macro code runs):

Database Error
  ('28000', "[28000] [Microsoft][ODBC Driver 18 for SQL Server][SQL Server]Couldn't complete the operation due to a system update. Close out this connection, sign in again, and retry the operation. (18456) (SQLDriverConnect)")

Evidence this is a known flake, not a real failure:

• The error happens at ODBC connection time, before the macro is parsed/executed.
• test (sqlserver) (same dialect family) passed.
• All 12 other warehouse jobs passed (snowflake, bigquery, redshift, postgres, clickhouse, databricks, spark, trino, vertica, duckdb, athena, dremio).
• The most recent failed run on master (April 28, run 25041506483) hit test (fabric) + test (snowflake) + test (redshift) simultaneously — classic transient warehouse availability pattern.

I don't have permission to rerun the failed job from this PR (fork). Happy to push an empty commit to retrigger if you'd like a clean run.

[MikaKerman]

One thing I noticed: when disable_samples=true, the macro sets test_result_rows_agate = {} which skips the expensive test_result_rows table query (the main goal). But _process_raw_test_results still calls get_test_rows_sample(test.result_rows, ...) for fail/warn tests -- and get_test_rows_sample checks the legacy_result_rows parameter first (the result_rows JSON column on elementary_test_results). If that column has data on older installs, samples would still appear even with disable_samples=true.

For a clean contract, might be worth passing disable_samples into _process_raw_test_results and gating the entire get_test_rows_sample call:

{% if not disable_samples and test.invocations_rank_index == 1 %}
    ...
    {% set test_rows_sample = elementary_cli.get_test_rows_sample(...) %}
{% endif %}

This way disable_samples=true means no sample data at all -- not just "skip the table query." Probably doesn't matter for cloud (the legacy column should be empty on modern syncs), but it's a tighter guarantee.

[devin-ai-integration]

@MikaKerman good catch, confirmed. get_test_rows_sample checks legacy_result_rows first (here), so samples from the elementary_test_results.result_rows JSON column would still leak through even with test_result_rows_agate = {}.

Fixed in 0e96bc3b: threaded disable_samples into _process_raw_test_results and gated the get_test_rows_sample call on not disable_samples. Now disable_samples=true means no sample data at all, not just "skip the table query".

[MikaKerman]

Another thing: even with disable_samples=true, the main query in all three variants still selects elementary_test_results.result_rows (via current_tests_run_results_query). That column is the legacy sample storage -- on modern installs it should be NULL so the I/O cost is negligible. But it's still selected, materialized into the temp table, read back via SELECT *, and serialized through agate_to_dicts.

For a fully clean implementation, current_tests_run_results_query could accept a parameter to select NULL as result_rows instead when samples are disabled. That said, this is a minor optimization on top of the main win (skipping the 522 GB test_result_rows table query), so it could also be a follow-up.

[devin-ai-integration]

@MikaKerman good point — also gated in 3507c3af:

• current_tests_run_results_query now accepts disable_samples; when true, it emits null as result_rows instead of elementary_test_results.result_rows. Both default__ and fabric__ callers pass it through.
• The clickhouse__ variant has its own inline INSERT query; gated the etr.result_rows selection there too (uses '' since the ClickHouse result_rows String column is NOT NULL).

Now with disable_samples=true the result_rows column is never read from elementary_test_results, never materialized into ordered_test_results, and never serialized through agate_to_dicts.

[devin-ai-integration]

CI on 3507c3af: 20/21 passed, only optional test (fabric) failed — same Azure SQL transient ODBC flake as last time:

Database Error
  ('28000', "[28000] [Microsoft][ODBC Driver 18 for SQL Server][SQL Server]Couldn't complete the operation due to a system update. Close out this connection, sign in again, and retry the operation. (18456) (SQLDriverConnect)")

This fails at SQLDriverConnect during dbt parse, before any macro under test runs. test (sqlserver) (also ODBC) and test (clickhouse) (the variant with the inline result_rows change) both pass.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@elementary/monitor/dbt_project/macros/get_test_results.sql`:
- Line 185: The ClickHouse macro clickhouse__get_test_results calls
_process_raw_test_results with elementary_tests_allowlist_status but never
initializes it in the ClickHouse path; initialize
elementary_tests_allowlist_status (e.g., to an empty array/object consistent
with other DB paths) before the post-processing call so
_process_raw_test_results always receives a defined value and sample selection
for non-dbt_test rows is not skipped; update the clickhouse__get_test_results
macro to set elementary_tests_allowlist_status (same name) prior to invoking
_process_raw_test_results.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8b190d45-2615-4b71-a58f-f287aad64be7

📥 Commits

Reviewing files that changed from the base of the PR and between 3507c3a and a174c35.

📒 Files selected for processing (5)

• elementary/monitor/api/report/report.py
• elementary/monitor/api/tests/tests.py
• elementary/monitor/dbt_project/macros/base_queries/current_tests_run_results_query.sql
• elementary/monitor/dbt_project/macros/get_test_results.sql
• elementary/monitor/fetchers/tests/tests.py