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 metric alias to sl api #6775

Open
wants to merge 10 commits into
base: current
Choose a base branch
from
Open

add metric alias to sl api #6775

wants to merge 10 commits into from

Conversation

mirnawong1
Copy link
Contributor

@mirnawong1 mirnawong1 commented Jan 15, 2025

this pr adds new info that you can query metric aliases in the jdbc and graph api for the semantic layer.

updated jdbc and graphql api doc, as well as added a release note entry

linear: https://linear.app/dbt-labs/issue/SL-3245/support-metric-aliases#comment-ab55cb17


🚀 Deployment available! Here are the direct links to the updated files:

@mirnawong1 mirnawong1 requested a review from a team as a code owner January 15, 2025 13:40
Copy link

vercel bot commented Jan 15, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs-getdbt-com ✅ Ready (Inspect) Visit Preview Jan 17, 2025 5:27pm

@github-actions github-actions bot added content Improvements or additions to content Docs team Authored by the Docs team @dbt Labs size: medium This change will take up to a week to address and removed Docs team Authored by the Docs team @dbt Labs labels Jan 15, 2025
website/docs/docs/dbt-cloud-apis/sl-graphql.md Outdated Show resolved Hide resolved
website/docs/docs/dbt-cloud-apis/sl-jdbc.md Outdated Show resolved Hide resolved
website/docs/docs/dbt-cloud-apis/sl-jdbc.md Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Docs team Authored by the Docs team @dbt Labs label Jan 15, 2025
@github-actions github-actions bot added size: large This change will more than a week to address and might require more than one person and removed size: medium This change will take up to a week to address labels Jan 15, 2025
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.

The metric alias stuff looks great!! I left some comments on the other parts of this page where I noticed some inconsistencies.

website/docs/docs/dbt-cloud-apis/sl-graphql.md Outdated Show resolved Hide resolved
website/docs/docs/dbt-cloud-apis/sl-graphql.md Outdated Show resolved Hide resolved
website/docs/docs/dbt-cloud-apis/sl-graphql.md Outdated Show resolved Hide resolved
website/docs/docs/dbt-cloud-apis/sl-jdbc.md Show resolved Hide resolved
@@ -265,6 +267,7 @@ createQuery(
```graphql
MetricInput {
name: String!
alias: String!
Copy link
Contributor Author

@mirnawong1 mirnawong1 Jan 16, 2025

Choose a reason for hiding this comment

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

@@ -403,6 +325,19 @@ select * from {{
}}
```

### Query by all dimensions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@courtneyholcomb added query by all dimensions from @Jstein77 duplicate PR

however should it be semantic_layer.query_with_all_group_bys or semantic_layer.group_by_all()? the example doesn't match the text above it?

Copy link

@serramatutu serramatutu Jan 16, 2025

Choose a reason for hiding this comment

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

@mirnawong1 it's semantic_layer.query_with_all_group_bys

I'm wondering if this should be documented at all, though? This was an ask by Sigma, and we decided to put it into a separate method instead of just query because that's not really the intended way of using the Semantic Layer, and there is a caveat to it: you can (possibly) get incorrect data if you don't know what you're doing and reaggregate metrics that aren't supposed to be reaggregated.

IMO regular users should just stick to query() and we shouldn't promise stability and/or backwards compatibility to query_with_all_group_bys() in the long run.

I think we should also hear Courtney's and Jordan's thoughts on this, though.

Copy link
Contributor Author

@mirnawong1 mirnawong1 Jan 16, 2025

Choose a reason for hiding this comment

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

sounds good, thank you! i copied it over from Jordan's pr so would be great to get clarity!

Copy link
Contributor

Choose a reason for hiding this comment

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

@serramatutu I think we should document it! It's a useful endpoint that can simplify certain query requests for users. I also don't think there's much downside in putting documenting it.

| 2023-12-01 | 1500.75 |
| 2023-12-02 | 1725.50 |
| 2023-12-03 | 1850.00 |
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Hiya @mirnawong1

Probs just me being picky but the table appears mis-aligned. Should the "|" be shifted to the right a bit?

Kind Regards
Natalie

Copy link
Contributor

@nataliefiann nataliefiann left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@nataliefiann nataliefiann left a comment

Choose a reason for hiding this comment

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

Hiya @mirnawong1

Ignore the first comment I made lol. I submitted the post too quickly.
I've approved this for you and left one question / suggestion

Kind Regards
Natalie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Improvements or additions to content Docs team Authored by the Docs team @dbt Labs size: large This change will more than a week to address and might require more than one person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants