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

Use helper function to find matching instances in dataset #1538

Merged
merged 2 commits into from
Dec 9, 2024

Conversation

courtneyholcomb
Copy link
Contributor

@courtneyholcomb courtneyholcomb commented Nov 20, 2024

Clean up. Just reducing some repeated code in dataflow to SQL logic.

@cla-bot cla-bot bot added the cla:yes label Nov 20, 2024
time_dimension_spec: TimeDimensionSpec,
) -> ColumnAssociation:
"""Given the name of the time dimension, return the set of columns associated with it in the data set."""
def instances_for_time_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.

The pluralized version of this is not currently used but will be used further up the stack.

@dbt-labs dbt-labs deleted a comment from github-actions bot Nov 20, 2024
@courtneyholcomb courtneyholcomb marked this pull request as ready for review November 21, 2024 00:02
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.

LGTM but see comment.

return column_associations_to_return[0]
def instance_for_time_dimension(self, time_dimension_spec: TimeDimensionSpec) -> TimeDimensionInstance:
"""Given the name of the time dimension, return the instance associated with it in the data set."""
return self.instances_for_time_dimensions((time_dimension_spec,))[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

A check / error message for the number of instances returned by instances_for_time_dimensions would be helpful here.

@courtneyholcomb courtneyholcomb enabled auto-merge (squash) December 9, 2024 19:09
@courtneyholcomb courtneyholcomb merged commit dcf17cf into main Dec 9, 2024
11 checks passed
@courtneyholcomb courtneyholcomb deleted the court/simp0 branch December 9, 2024 19:13
plypaul pushed a commit that referenced this pull request Feb 6, 2025
Clean up. Just reducing some repeated code in dataflow to SQL logic.
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.

2 participants