Skip to content

Render simple-metric inputs without a dunder prefix for the WHERE filter#1941

Merged
plypaul merged 6 commits intomainfrom
p/where_filter_query_rewriting__02
Nov 20, 2025
Merged

Render simple-metric inputs without a dunder prefix for the WHERE filter#1941
plypaul merged 6 commits intomainfrom
p/where_filter_query_rewriting__02

Conversation

@plypaul
Copy link
Contributor

@plypaul plypaul commented Nov 20, 2025

This PR updates the SQL rendering for the WhereConstraintNode to use a subquery that generates column aliases without a dunder prefix for simple metric inputs e.g. (bookings instead of __bookings).

This is added to handle a migration case - some customers have been writing SQL strings in the filters that reference internal column names, which is an unsupported use case. However, to reduce errors during migration, this PR temporarily restores the previous behavior.

Due to the snapshot changes, please view by commit.

@cla-bot cla-bot bot added the cla:yes label Nov 20, 2025
@plypaul plypaul force-pushed the p/where_filter_query_rewriting__02 branch 2 times, most recently from 355527a to 75cc786 Compare November 20, 2025 01:15
@plypaul plypaul force-pushed the p/where_filter_query_rewriting__02 branch from 75cc786 to 9741731 Compare November 20, 2025 01:18
@plypaul plypaul marked this pull request as ready for review November 20, 2025 01:35
@plypaul plypaul requested a review from a team as a code owner November 20, 2025 01:35
@plypaul plypaul merged commit e3092ba into main Nov 20, 2025
13 checks passed
@plypaul plypaul deleted the p/where_filter_query_rewriting__02 branch November 20, 2025 09:14
SELECT
metric_time__day
, SUM(__visits) AS __visits
, SUM(visits) AS __visits
Copy link
Contributor

Choose a reason for hiding this comment

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

This works but it's definitely going to look confusing to users. I think we can release as-is but I wonder if we can improve this by rendering only the non-dundered name anytime after the initial source node. But @plypaul maybe you already looked into that and it didn't work?

plypaul added a commit that referenced this pull request Dec 2, 2025
This PR is an update to #1941 to handle an edge case with manifests that
contain a metric and an entity with the same name. Instead of aliasing
all simple-metrics, the change is to alias only a specific one that is
relevant to the containing branch in the dataflow plan. This limits
collisions in the `SELECT` statement that is generated from the branch.

Due to the large snapshot changes, please view by commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants