Skip to content

bound dimensions on metric nodes#622

Merged
CircArgs merged 3 commits intoDataJunction:mainfrom
CircArgs:bound-dimensions
Jul 12, 2023
Merged

bound dimensions on metric nodes#622
CircArgs merged 3 commits intoDataJunction:mainfrom
CircArgs:bound-dimensions

Conversation

@CircArgs
Copy link
Copy Markdown
Contributor

@CircArgs CircArgs commented Jul 12, 2023

Summary

Adds an attribute to Metric Nodes that allow the author to specify that when the metric is used, some columns from the sourced node must be grouped by.

Test Plan

Unit test

@netlify
Copy link
Copy Markdown

netlify bot commented Jul 12, 2023

Deploy Preview for thriving-cassata-78ae72 canceled.

Name Link
🔨 Latest commit 06efc51
🔍 Latest deploy log https://app.netlify.com/sites/thriving-cassata-78ae72/deploys/64aeb839d6ccde00081dc0a3

@CircArgs CircArgs changed the title bound dimensions bound dimensions on metric nodes Jul 12, 2023
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks like you ran the revision twice, you should be able to just delete this file.

@samredai
Copy link
Copy Markdown
Contributor

@CircArgs VERY cool! It's awesome that the columns objects are actually linked in the metadata so we could probably very easily add in a check in the node update logic to prevent dropping a column in a transform if it's being used as a required dimension by any metrics.

@agorajek can you take a look at this when you get a chance and see how this works for some of the use cases you had in mind?

Copy link
Copy Markdown
Collaborator

@shangyian shangyian 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 adding! I think the data models here all make sense.

Will this additionally need to modify the POST /nodes/metric endpoint so that someone creating a metric can set the bound dimensions of the metric? And presumably that would be where you do the check for whether their provided bound_dimensions are actually valid.

),
)

# get dimension columns which are required
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does this need to additionally check if the user-provided dimensions overlap with the bound_dimensions set on the node?

@CircArgs
Copy link
Copy Markdown
Contributor Author

@shangyian

Will this additionally need to modify the POST /nodes/metric endpoint so that someone creating a metric can set the bound dimensions of the metric? And presumably that would be where you do the check for whether their provided bound_dimensions are actually valid.

Yeah I think that's a good place to do that validation. Can it be done in a follow-up PR?

@shangyian
Copy link
Copy Markdown
Collaborator

@CircArgs follow-up PR sounds good!

Copy link
Copy Markdown
Member

@agorajek agorajek left a comment

Choose a reason for hiding this comment

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

Thanks!

@CircArgs CircArgs merged commit 0d4f544 into DataJunction:main Jul 12, 2023
@CircArgs CircArgs mentioned this pull request Jul 13, 2023
3 tasks
youngman-droid pushed a commit to youngman-droid/dj that referenced this pull request Aug 26, 2023
* bound dimensions

* pylint can't read

* filter used dimensions from used bound dimensions
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.

4 participants