fix: make metric_stability run on BigQuery and ClickHouse (follow-up to #1050) - #1053
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
👋 @haritamar |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughAdds the ChangesMetric stability detection
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The adapter-specific BigQuery timestamp cast and synchronous ClickHouse mutation address the reported metric_stability failures without a demonstrated merge-blocking risk. Sequence Diagram(s)sequenceDiagram
participant IntegrationTest
participant MetricStability
participant MetricStabilityQuery
participant MetricHistory
IntegrationTest->>MetricStability: Load rows and run dbt test
MetricStability->>MetricStabilityQuery: Pass windows, baselines, and thresholds
MetricStabilityQuery->>MetricHistory: Read persisted measurements
MetricHistory-->>MetricStabilityQuery: Return bucket versions
MetricStabilityQuery-->>MetricStability: Return exceeding metrics
MetricStability-->>IntegrationTest: Report test result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 1 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
- Cast the settled-measurement lower bound to a timestamp: on BigQuery edr_timeadd returns a DATE for week/month/quarter/year parts, so comparing it against the TIMESTAMP updated_at column failed. - ClickHouse has no plain UPDATE; the settling test now issues an ALTER TABLE ... UPDATE mutation (synchronously) on that target. Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
779dda1 to
ea2bb20
Compare
54b9725 to
e4c9516
Compare
Summary
Fixes the two CI failures on #1050 (this branch is #1050 + one commit on top; it can be merged as-is or the commit cherry-picked into #1050).
BigQuery –
test_metric_stability_detects_restatement_with_weekly_bucketserrored withNo matching signature for operator >= for argument types: TIMESTAMP, DATE.bigquery__edr_timeaddreturns aDATEforweek/month/quarter/yearparts (it usesdate_add(cast(... as date), ...)), so the settled-measurement bound compared aTIMESTAMPcolumn against aDATE:Same pattern as
get_start_bucket_in_data. Day-based ages were unaffected, which is why only the weekly test caught it.ClickHouse –
test_metric_stability_ignores_measurements_taken_while_settlingissues a plainUPDATEondata_monitoring_metrics, which ClickHouse rejects (Lightweight updates are not supported). On that target the test now runsALTER TABLE ... UPDATE ... SETTINGS mutations_sync = 1instead (synchronous so the next run sees the mutated rows).Verified locally: both tests pass on postgres; the settling test passes on ClickHouse. No BQ credentials locally — relying on CI here.
The remaining
test_dbt_artifacts/test_artifacts.py::test_timingsfailures on the fusion jobs in #1050 are unrelated to this feature (a separate fix landed viadevin/1788692120-fusion-timing-datetime).Link to Devin session: https://app.devin.ai/sessions/2c8e65abe19f4bae97f105d9638d1065
Open in Devin Desktop: https://app.devin.ai/desktop/session/2c8e65abe19f4bae97f105d9638d1065?variant=devin
Requested by: @haritamar
Summary by CodeRabbit
New Features
Tests