fix: apply column expression on freshness check correctly#2607
Conversation
|
There was a problem hiding this comment.
Pull request overview
Fixes freshness checks to correctly honor column_expression (and only default to the configured column when no expression is provided), aligning freshness behavior with other check types that already support column-expression overrides.
Changes:
- Update
FreshnessCheckImpl.column_expressionto prefer the base check’s resolvedcolumn_expressionand fall back toCOLUMN(check_yaml.column)when absent. - Adjust the column-expression feature test to ensure freshness actually requires the expression (by extracting the timestamp from JSON metadata).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
soda-tests/tests/feature/test_column_expression.py |
Updates test data + contract YAML to validate freshness uses column_expression (JSON extract) rather than the raw column value. |
soda-core/src/soda_core/contracts/impl/check_types/freshness_check.py |
Fixes freshness check column-expression resolution by using the standard check-level expression mechanism with a safe fallback. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| column: metadata | ||
| column_expression: '${{var.metadata_col_expr}}' |
There was a problem hiding this comment.
Is this valid in a contract? Thinking about it I would see these as conflicting, so perhaps we should reject it in JSON schema validation?
There was a problem hiding this comment.
are you referring to having both column and column_expression present? Yeah this is valid, at least for now. We didn't want to do a breaking change of removing column which may potentially become obsolete. Not allowing both is an option, but no particular reason to do it. This still allows us to associate the check with a column.
| if not expression: | ||
| expression = COLUMN(self.check_yaml.column) |
There was a problem hiding this comment.
Do we also need to check whether self.check_yaml.column is set? What if neither is available?
There was a problem hiding this comment.
nah self.check_yaml is FreshnessCheckYaml type which does mandatory column read self.column: str = check_yaml_object.read_string("column")
Description
Checklist