Skip to content

Qualify segment filters in CTEs#55

Merged
nicosuave merged 3 commits intomainfrom
nicosuave/count-distinct-cte
Jan 8, 2026
Merged

Qualify segment filters in CTEs#55
nicosuave merged 3 commits intomainfrom
nicosuave/count-distinct-cte

Conversation

@nicosuave
Copy link
Copy Markdown
Member

Qualifies unaliased columns in segment filters so they push down into model CTEs. Adds a regression test for count_distinct with a segment lacking a {model} placeholder.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 70234b20b2

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread sidemantic/sql/generator.py Outdated
Comment on lines +445 to +451
parsed = sqlglot.parse_one(filter_sql, dialect=self.dialect)
except Exception:
return filter_sql

for col in parsed.find_all(exp.Column):
if not col.table:
col.set("table", model_alias)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid qualifying columns inside subqueries

This blanket qualification rewrites every unqualified column, including ones inside subqueries. For segment SQL like id IN (SELECT id FROM other_table), the inner id is now forced to model_alias.id, which can make the subquery invalid or change its semantics (it no longer refers to other_table.id). This only happens when segment filters include subqueries or unqualified inner columns, but it will silently corrupt those filters. Consider limiting qualification to the top-level scope or skipping columns within nested SELECTs.

Useful? React with 👍 / 👎.

@nicosuave nicosuave merged commit 50a54dd into main Jan 8, 2026
10 checks passed
@nicosuave nicosuave deleted the nicosuave/count-distinct-cte branch January 8, 2026 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant