fix(core): qualify check_filter columns in valid_reference_data JOIN (DTL-1517) - #2679
Open
paulteehan wants to merge 5 commits into
Open
fix(core): qualify check_filter columns in valid_reference_data JOIN (DTL-1517)#2679paulteehan wants to merge 5 commits into
paulteehan wants to merge 5 commits into
Conversation
A check-level filter on a valid_reference_data check produced ambiguous SQL whenever the filter referenced a column that also exists in the reference dataset (e.g. WHERE (id > 1) when both source and reference tables have an id column). Most warehouses (DuckDB, Postgres, Snowflake, ...) reject this with an "ambiguous reference" binder error and the check ends up NOT_EVALUATED. The same filter string is reused by the optimized aggregate row-count query (RowCountMetricImpl + AggregationQuery), which runs against the source CTE alone with no JOIN and no alias on the FROM. So we cannot simply prefix the filter at the metric level: a literal "C".id > 1 filter would break the aggregate query. Fix: qualify unqualified column references inside the filter only when rendering the InvalidReferenceCountQuery JOIN, keeping the metric's check_filter string untouched. The new helper uses sqlglot (already a dependency) to walk exp.Column nodes and set the source alias on any column without one — already-qualified columns, identifiers inside string literals, and function names are left alone. Falls back to the unmodified filter on parse errors so the database error surfaces as it did before. Adds an integration test that exercises a reference table sharing an id column with the source, with a check filter on id.
The reference-check filter rewrite relies on a round-trip through sqlglot (parse -> qualify columns -> render). For data sources whose SqlDialect sets sqlglot_dialect="" (DB2 LUW and z/OS do this because sqlglot has no native DB2 dialect) the round-trip would fall back to sqlglot's default parser/printer, which can mangle DB2-specific syntax in filter expressions (timestamp literals like '2024-01-01-12.30.00.000000', CURRENT TIMESTAMP - 1 DAY, etc.). Short-circuit when read_dialect is empty/None and return the filter unchanged, preserving the pre-fix behaviour for those data sources. The original ambiguous-column bug remains there for now; it can be fixed separately once sqlglot adds a DB2 dialect or a hand-rolled column resolver is added.
…ialect Commit 703fa25 added a defensive `if not read_dialect: return sql_expression` guard to qualify_unqualified_columns_with_alias because at that time DB2 LUW and z/OS set sqlglot_dialect="" (no native sqlglot dialect — round-tripping DB2 SQL through sqlglot's default parser/printer can mangle DB2-specific syntax). The guard kept DB2 on the pre-fix code path: the ambiguous-column bug from DTL-1517 still surfaced on DB2, but no new mangling regression was introduced. soda-extensions now ships a native DB2 sqlglot dialect (registered as "db2" in soda_db2.base.sqlglot_dialect; both LUW and z/OS adapters set SQLGLOT_DIALECT="db2"). With every Soda data source declaring a real sqlglot dialect, the guard has nothing left to protect — drop it. Refresh the docstring to spell out the contract (every adapter must pass a real sqlglot dialect name in `sqlglot_dialect=`) so future adapters get a deliberate rather than accidental fix.
Switch from always-qualify to retry-on-error. The first execution emits the user's check_filter byte-for-byte; only if the JOIN query fails do we retry once with the filter columns qualified against the source alias. If that retry also fails, both errors are surfaced (initial query + initial error, retry query + retry error) so the user is not misled about what actually went wrong. This avoids two surprises from the previous always-qualify approach: - sqlglot round-trip canonicalisation of every filter expression - snapshot churn on Athena/Databricks for tests whose filter is unambiguous Engine error-message wording for ambiguous-column errors varies across the ~15 supported databases; rather than maintaining a per-engine substring list, the retry fires on any error from InvalidReferenceCountQuery.execute and lets the existing per-engine integration test prove the fix works on each adapter. Also fold in two helper improvements flagged in review: - qualify_unqualified_columns_with_alias now skips columns inside nested SELECT scopes (subqueries, EXISTS bodies, scalar subqueries) so they resolve in their own scope rather than being silently aliased to the outer one. - Broaden the exception fallback to ``except Exception`` so unknown sqlglot dialects (which raise ValueError, not ParseError) also fall back to the un-rewritten input. Adds 11 unit tests in test_sql_utils.py covering the helper directly, including dialect-specific quoting across postgres / snowflake / duckdb / trino / athena / bigquery / databricks / tsql. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Niels-b
requested changes
Apr 29, 2026
Niels-b
left a comment
Niels-b
left a comment
Contributor
There was a problem hiding this comment.
Some remarks that I'd like to see discussed/addressed.
| except Exception as e: | ||
| logger.error(msg=f"Could not execute invalid reference query {self.sql}: {e}", exc_info=True) | ||
| return [] | ||
| except Exception as initial_error: |
Contributor
There was a problem hiding this comment.
This will make the retry trigger for any error, even if the connection is closed (or something like that).
Comment on lines
+364
to
+382
| retry_sql: str = self._build_qualified_filter_sql() | ||
| try: | ||
| query_result = self.data_source_impl.execute_query(retry_sql) | ||
| except Exception as retry_error: | ||
| logger.error( | ||
| msg=( | ||
| f"Could not execute invalid reference query.\n" | ||
| f"Initial query failed: {self.sql}\n" | ||
| f" Initial error: {initial_error}\n" | ||
| f"Retried with the check_filter columns qualified against source alias " | ||
| f"{self.referencing_alias!r} (common fix for ambiguous-column errors when " | ||
| f"the filter references a column that exists in both the source and " | ||
| f"reference datasets); the retry also failed.\n" | ||
| f"Retry query: {retry_sql}\n" | ||
| f" Retry error: {retry_error}" | ||
| ), | ||
| exc_info=True, | ||
| ) | ||
| return [] |
Contributor
There was a problem hiding this comment.
Is there a way that we can detect this beforehand? Not really the biggest fan of "try, error, retry" if we can avoid it. These things show up in DB logs. The error message itself is clear. But what if it's not related to the filter? Then we're still showing this error in call cases
|
|
||
| return query | ||
|
|
||
| def _build_qualified_filter_sql(self) -> str: |
Contributor
There was a problem hiding this comment.
Name is confusing to me, does this build only the filter or the full SQL? (Yes it's in the docs below 😉 ).
Maybe _build_query_with_qualified_filter?
Comment on lines
+55
to
+58
| Note: a successful parse round-trips through sqlglot's printer, which canonicalises some | ||
| syntax (e.g. ``col::int`` becomes ``CAST(col AS INT)``, function names get uppercased). | ||
| On the same read/write dialect the rewrite is semantically equivalent in standard cases, | ||
| but recognised functions may be rewritten into canonical equivalents. |
Contributor
There was a problem hiding this comment.
Should we also add some test cases for this?
Contributor
|
Thanks for picking this up @paulteehan! Sorry for posting a Claude-generated comment but I have some concerns about the approach I am not sure those alternatives are 100% better or the way to go, but the silent assumption to qualify with C we are adding here worries me |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes DTL-1517.
A check-level `filter:` on a `valid_reference_data` check produced ambiguous SQL whenever the filter referenced a column that also exists in the reference dataset. Concrete shape:
```yaml
columns:
valid_reference_data:
dataset: //valid_countries
column: country_code
checks:
filter: |
id > 1 #
idexists in both source AND reference table```
The reference-check `InvalidReferenceCountQuery` rendered the filter un-aliased:
```sql
SELECT COUNT(*)
FROM "_soda_filtered_dataset" AS "C"
LEFT JOIN "_soda_filtered_referenced_dataset" AS "R" ON "C"."country" = "R"."country_code"
WHERE (id > 1) -- ambiguous: id is in both C and R
AND NOT("C"."country" IS NULL) AND "R"."country_code" IS NULL;
```
DuckDB rejects with `Binder Error: Ambiguous reference to column name "id"`. Postgres / Snowflake / DB2 all produce the same error. The check ends up `NOT_EVALUATED`.
A previous attempt at this fix (#2535, closed without merging) renamed the JOIN aliases from `C` / `R` to `TARGET` / `REFERENCE` and required users to manually qualify their filter columns. That approach was abandoned — Milan confirmed in the recent Slack thread that "this is known and there is no current workaround." This PR takes a different, transparent approach: filters keep their plain source-only column references, and we automatically qualify them via sqlglot when rendering the JOIN query.
How
The metric's `self.check_filter` string is left untouched: the optimized aggregate row-count query (`RowCountMetricImpl.sql_expression` rendered into `AggregationQuery.build_sql`) runs against the source CTE alone with no JOIN and no `AS "C"` on the FROM, so a literal `"C".id > 1` filter would not resolve there.
Why sqlglot over a regex
The filter can reference any source column, not just the check's column. We don't always have an enumerated column list — the contract YAML may not list every column. sqlglot is dialect-aware and skips identifiers inside string literals, function names, etc.
DB2 — companion change in soda-extensions
DB2 LUW and z/OS do not have a sqlglot dialect. A companion soda-extensions PR adds a native DB2 sqlglot dialect (registered as `"db2"`). The DB2 PR can be reviewed independently — this one is correct on every adapter today regardless.
Test plan
Note for reviewers
This was AI-written. Bug analysis, fix design, and tests are mine; please scrutinise.
🤖 Generated with Claude Code