Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for querying with metric alias #1573

Merged
merged 12 commits into from
Jan 10, 2025

Conversation

serramatutu
Copy link
Contributor

@serramatutu serramatutu commented Dec 13, 2024

Summary

This PR introduces support for querying with aliased metrics. This is done by adding an AliasSpecsNode after the order by step in DataflowPlanBuilder. To make the alias information reach that point, I also had to modify some of the query interface classes to allow aliases in metrics.

You can review commit by commit.

@cla-bot cla-bot bot added the cla:yes label Dec 13, 2024
@serramatutu serramatutu force-pushed the serramatutu/metric-alias branch 2 times, most recently from 42b8dcf to 0fbefff Compare December 13, 2024 10:01
@serramatutu serramatutu added the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Dec 13, 2024
@dbt-labs dbt-labs deleted a comment from github-actions bot Dec 13, 2024
Copy link
Contributor

@courtneyholcomb courtneyholcomb left a comment

Choose a reason for hiding this comment

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

Thanks for handling this!! Dataflow plan looks good. There are some changes that need to be made to the other parts of the code, and I don't think you should try to make them before you leave for vacation, so I can pick this up!

@courtneyholcomb courtneyholcomb force-pushed the serramatutu/metric-alias branch 3 times, most recently from 12a2463 to 06eada3 Compare December 20, 2024 15:34
Copy link
Contributor

@plypaul plypaul left a comment

Choose a reason for hiding this comment

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

  • Since the alias in this case is describing user inputs, can it be added as a field to ResolverInputForMetric?
  • If not, need to have a think about the placement as the spec pattern is more about identifying items in the semantic manifest.
  • Can you add a test case for when the user specifies two aliases that are the same in a query?

@courtneyholcomb courtneyholcomb force-pushed the serramatutu/metric-alias branch from 86a7499 to 76b1c03 Compare December 20, 2024 19:59
@WilliamDee WilliamDee force-pushed the serramatutu/metric-alias branch from 75d0e42 to ea952ef Compare January 7, 2025 21:06
@WilliamDee WilliamDee force-pushed the serramatutu/metric-alias branch from ea952ef to dfc4743 Compare January 9, 2025 01:10
Copy link
Contributor

@courtneyholcomb courtneyholcomb left a comment

Choose a reason for hiding this comment

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

LGTM!

@courtneyholcomb courtneyholcomb merged commit a4cc831 into main Jan 10, 2025
11 checks passed
@courtneyholcomb courtneyholcomb deleted the serramatutu/metric-alias branch January 10, 2025 21:48
@github-actions github-actions bot removed the Run Tests With Other SQL Engines Runs the test suite against the SQL engines in our target environment label Jan 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants