Fix retention and conversion filters not resolving dimension names#125
Fix retention and conversion filters not resolving dimension names#125
Conversation
Filters passed to retention, conversion, and multistep conversion queries were only stripped of model prefixes (events.city -> city) but not resolved to their SQL expressions. This caused DuckDB errors when dimension names don't match raw column names (e.g., city is actually json_extract_string(properties, '$.city')). Adds _resolve_filter_dimensions() helper that rewrites dimension names in filter expressions to their actual SQL via sqlglot. Applied after _strip_model_prefixes in all three generators. Also fixes the legacy 2-step conversion generator which was ignoring filters entirely.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b7c943b85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| replacement = sqlglot.parse_one(dim_map[column.name], dialect=self.dialect) | ||
| column.replace(replacement) |
There was a problem hiding this comment.
Replacing a filter column with dim.sql_expr verbatim here can reintroduce model-qualified references (for example events.region) that _strip_model_prefixes just removed. In conversion/funnel generation, those rewritten filters are emitted against FROM (<model.sql>) AS t (and AS s), so a request filter like region = 'US' can become events.region = 'US' and fail because events is not in scope. This turns previously valid filters into invalid SQL for SQL-backed models whose dimensions are qualified by model name.
Useful? React with 👍 / 👎.
When _resolve_filter_dimensions replaces a dimension name with its sql_expr, the expression may contain model-qualified references (e.g., events.region) that _strip_model_prefixes already removed. These leak into FROM (<sql>) AS t subqueries where the model name is not a valid table reference. Strip them in the replacement the same way _strip_model_prefixes does.
) * Fix retention and conversion filters not resolving dimension names Filters passed to retention, conversion, and multistep conversion queries were only stripped of model prefixes (events.city -> city) but not resolved to their SQL expressions. This caused DuckDB errors when dimension names don't match raw column names (e.g., city is actually json_extract_string(properties, '$.city')). Adds _resolve_filter_dimensions() helper that rewrites dimension names in filter expressions to their actual SQL via sqlglot. Applied after _strip_model_prefixes in all three generators. Also fixes the legacy 2-step conversion generator which was ignoring filters entirely. * Strip model-qualified refs from resolved filter dimension expressions When _resolve_filter_dimensions replaces a dimension name with its sql_expr, the expression may contain model-qualified references (e.g., events.region) that _strip_model_prefixes already removed. These leak into FROM (<sql>) AS t subqueries where the model name is not a valid table reference. Strip them in the replacement the same way _strip_model_prefixes does.
1 participant
Summary
city) were not resolved to their SQL expressions (e.g.,json_extract_string(properties, '$.city'))_resolve_filter_dimensions()helper that rewrites dimension names via sqlglot_generate_retention_query,_generate_multistep_conversion_query, and_generate_conversion_query