Conversation
4cf77c3 to
e1ad7e7
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a crash in MetricQuery.execute() when a metric check query returns zero rows. Previously, the code would crash with an IndexError when attempting to access query_result.rows[0][0] on an empty result set. The fix adds a check for empty rows and returns a Measurement with value=None instead, allowing the threshold evaluation to produce a NOT_EVALUATED outcome gracefully.
Changes:
- Added empty row check in
MetricQuery.execute()to preventIndexErrorwhen queries return no rows - Added warning log when metric queries return empty results for better observability
- Added comprehensive unit tests covering both empty and non-empty query results
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| soda-core/src/soda_core/contracts/impl/check_types/metric_check.py | Added conditional check for empty rows and logging before accessing query result |
| soda-tests/tests/unit/test_metric_check.py | Added unit tests for both empty and non-empty metric query result scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When a metric check query returns 0 rows (e.g. a non-aggregate query with a restrictive WHERE clause), MetricQuery.execute() crashed with IndexError at `query_result.rows[0][0]`. Now checks for empty rows, logs a warning, and returns a Measurement with value=None so threshold evaluation marks the check as NOT_EVALUATED instead of crashing with an unhandled exception. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
e1ad7e7 to
64f7dbb
Compare
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Niels-b
left a comment
There was a problem hiding this comment.
LGTM!
Nice work with the tests for this. I like the idea of testing specific features with this.
But we should be careful not to "over-MagicMock" everything. Integration tests (on a Postgres) will still be required imo.
When a metric check query returns 0 rows (e.g. a non-aggregate query with a restrictive WHERE clause), MetricQuery.execute() crashed with IndexError at
query_result.rows[0][0].Now checks for empty rows, logs a warning, and returns a Measurement with value=None so threshold evaluation produces a NOT EVALUATED outcome with a clear reason instead of an unhandled crash.
Description
Checklist