Skip to content

Conversation

@devin-ai-integration
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Jan 27, 2026

Summary

Re-enables ClickHouse testing in the CLI test workflows. This was previously disabled and the changes include:

  1. Uncommented the "Start Clickhouse" step in test-warehouse.yml
  2. Added clickhouse to the workflow_dispatch options in test-warehouse.yml
  3. Added clickhouse to the warehouse-type matrix in test-all-warehouses.yml
  4. Added clickhouse to the "Seed e2e dbt project" step condition (since ClickHouse runs in Docker like Postgres)

Updates since last revision

Fixed a "Object of type Undefined is not JSON serializable" error in the populate_test_alerts macro that was causing the "Run monitor" step to fail for ClickHouse.

Root cause: ClickHouse returns columns with table prefixes (e.g., failed_tests.database_name instead of database_name) when column names are ambiguous across joined tables. The affected columns (database_name, schema_name, tags, test_params, severity, status, result_rows) exist in both the failed_tests CTE and the tests table (from dbt_tests).

Fix: Added explicit as column_name aliases in the SQL query for the ambiguous columns. This forces ClickHouse to use the alias name instead of the qualified column name.

Review & Testing Checklist for Human

  • Critical: Verify the SQL alias changes in test_alerts.sql don't break other warehouses - CI shows postgres, bigquery, and databricks_catalog passed, but please verify the query behavior is unchanged
  • Manually trigger the ClickHouse workflow to verify all steps pass (ClickHouse test passed when manually triggered during development)
  • Confirm CI_PROFILES_YML secret contains valid clickhouse credentials

Recommended Test Plan

  1. Review the SQL alias changes in test_alerts.sql - the aliases are redundant for most warehouses but necessary for ClickHouse
  2. Verify postgres CI passed (confirms the SQL changes don't break existing functionality)
  3. Manually trigger the ClickHouse workflow after merge to confirm it works in the full CI matrix

Notes

Linear ticket: ELE-5219

Link to Devin run: https://app.devin.ai/sessions/b483336a95be422d95bfcafbdf1cbb51
Requested by: Itamar Hartstein (@haritamar)

Summary by CodeRabbit

  • New Features

    • ClickHouse added as a selectable warehouse in test workflows.
  • Tests

    • CI matrix and per-workflow inputs extended to exercise ClickHouse.
  • Chores

    • Workflows now start ClickHouse when selected and include it in seeding/run conditions.
    • Workflow runs enable additional debug output.
  • Bug Fixes

    • Alert processing made more resilient to varying or missing field names and prefixed keys.

✏️ Tip: You can customize this high-level summary in your review settings.

Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
@linear
Copy link

linear bot commented Jan 27, 2026

@devin-ai-integration
Copy link
Contributor Author

🤖 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
Copy link
Contributor

👋 @devin-ai-integration[bot]
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
Copy link

coderabbitai bot commented Jan 27, 2026

📝 Walkthrough

Walkthrough

Adds clickhouse to CI workflows (matrix and workflow_dispatch), enables the ClickHouse startup step and seed/run conditions in CI, sets DBT_EDR_DEBUG=1 for the run step, and changes the DBT macro to use safe .get() accessors with fallback keys for alert fields.

Changes

Cohort / File(s) Summary
CI: Workflows (ClickHouse enablement)
\.github/workflows/test-all-warehouses.yml, \.github/workflows/test-warehouse.yml
Adds clickhouse to the warehouse-type matrix and workflow_dispatch inputs; enables the "Start Clickhouse" docker-compose step when warehouse-type is clickhouse; expands seed/trigger conditions to include clickhouse; sets DBT_EDR_DEBUG=1 in the run monitor step.
DBT macro: robust alert field access
elementary/monitor/dbt_project/macros/alerts/population/test_alerts.sql
Replaces direct property access on raw_test_alert with .get() fallbacks and alternative keys (handles prefixed/missing columns and variants like failed_tests.*); adjusts status/test_type initialization and test_rows_sample retrieval; preserves existing control flow and object construction.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰
I nudged a CI matrix, tiny whiskers in the night,
Woke ClickHouse gently — tests prepared for flight.
I swapped fragile looks for patient .get() cheer,
Now alerts find their keys, no mystery here! 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main changes: re-enabling ClickHouse testing in CLI test workflows by uncommenting steps and adding ClickHouse to matrix configurations.
Linked Issues check ✅ Passed All objectives from ELE-5219 are met: ClickHouse is uncommented in test-warehouse.yml, added to workflow_dispatch options, added to the warehouse-type matrix in test-all-warehouses.yml, and seeding condition updated.
Out of Scope Changes check ✅ Passed The changes to test_alerts.sql for handling ClickHouse column name variations are directly related to the objective of ensuring ClickHouse runs successfully and represents necessary fixes beyond just re-enabling tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing touches
  • 📝 Generate docstrings

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

Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In @.github/workflows/test-warehouse.yml:
- Around line 107-110: The workflow currently starts the clickhouse service with
"docker compose up -d clickhouse" but doesn't wait for it to be ready; either
add a healthcheck to the clickhouse service definition (a simple CMD that runs a
clickhouse-client query or TCP check) or add a follow-up workflow step after the
"Start Clickhouse" step that polls the clickhouse service until it's ready (for
example, loop running "docker compose exec clickhouse clickhouse-client --query
'SELECT 1'" with a timeout/retry and exit non-zero on failure). Update the
service named "clickhouse" or the workflow step that runs "docker compose up -d
clickhouse" accordingly so tests only proceed once ClickHouse accepts
connections.

haritamar and others added 16 commits January 27, 2026 17:01
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
…lickHouse column names

Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
…handle ClickHouse column names"

This reverts commit 5a7efb8.
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
…d detection

Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
ClickHouse returns columns with table prefix (e.g., 'failed_tests.database_name'
instead of 'database_name'). Use .get() with fallbacks to handle both naming
conventions for the affected fields:
- database_name
- schema_name
- tags
- test_params
- severity
- status
- result_rows

Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@elementary/monitor/dbt_project/macros/alerts/population/test_alerts.sql`:
- Around line 9-10: The fallback key used when setting test_type is incorrect:
update the raw_test_alert.get call in the test_type assignment (the line that
sets test_type via raw_test_alert.get('alert_type',
raw_test_alert.get('failed_tests.test_type'))) to use the actual output column
name produced by the query (failed_tests.alert_type) or simply 'alert_type' as
the fallback; modify the raw_test_alert.get fallback to
raw_test_alert.get('failed_tests.alert_type') so the lookup matches the aliased
column emitted by the query where failed_tests.test_type is aliased as
alert_type.
🧹 Nitpick comments (1)
elementary/monitor/dbt_project/macros/alerts/population/test_alerts.sql (1)

17-51: Consider documenting or unifying the field access pattern.

The comment on line 17 is helpful, but the selective application of .get() fallbacks raises questions. Some failed_tests columns use fallbacks (database_name, schema_name, tags, test_params, severity) while others use direct access (alert_id, table_name, column_name, test_name, etc.).

If this distinction is based on observed ClickHouse behavior during testing, consider expanding the comment to explain why only these specific columns need fallbacks. This will help future maintainers understand when to add fallbacks for new fields.

Comment on lines 9 to 10
{% set test_type = raw_test_alert.get('alert_type', raw_test_alert.get('failed_tests.test_type')) %}
{% set status = raw_test_alert.get('status', raw_test_alert.get('failed_tests.status')) | lower %}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Incorrect fallback key on line 9.

Looking at the SQL query, test_type is aliased to alert_type on line 149:

failed_tests.test_type as alert_type,

The fallback key 'failed_tests.test_type' will never match since the output column is named alert_type, not test_type. If ClickHouse were to prefix this column, it would be failed_tests.alert_type, not failed_tests.test_type.

Proposed fix
-        {% set test_type = raw_test_alert.get('alert_type', raw_test_alert.get('failed_tests.test_type')) %}
+        {% set test_type = raw_test_alert.get('alert_type', raw_test_alert.get('failed_tests.alert_type')) %}
🤖 Prompt for AI Agents
In `@elementary/monitor/dbt_project/macros/alerts/population/test_alerts.sql`
around lines 9 - 10, The fallback key used when setting test_type is incorrect:
update the raw_test_alert.get call in the test_type assignment (the line that
sets test_type via raw_test_alert.get('alert_type',
raw_test_alert.get('failed_tests.test_type'))) to use the actual output column
name produced by the query (failed_tests.alert_type) or simply 'alert_type' as
the fallback; modify the raw_test_alert.get fallback to
raw_test_alert.get('failed_tests.alert_type') so the lookup matches the aliased
column emitted by the query where failed_tests.test_type is aliased as
alert_type.

devin-ai-integration bot and others added 2 commits January 27, 2026 17:48
ClickHouse returns columns with table prefix when column names are ambiguous
across joined tables. The affected columns (database_name, schema_name, tags,
test_params, severity, status, result_rows) exist in both failed_tests and
tests tables.

Fix: Add explicit 'as column_name' aliases in the SQL query to force
ClickHouse to use the alias name instead of the qualified column name.
This is cleaner than using .get() fallbacks in the macro.

Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
@haritamar haritamar merged commit 1547a06 into master Jan 27, 2026
13 of 16 checks passed
@haritamar haritamar deleted the ele-5219-re-enable-clickhouse-in-cli-tests branch January 27, 2026 20:35
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.

2 participants