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

Set max_metric_default_time_granularity property on PushDownResult #1331

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

courtneyholcomb
Copy link
Contributor

Per title. Set this property when visiting the metric node and narrow down to the max when visiting a query node.

@courtneyholcomb courtneyholcomb requested a review from plypaul July 12, 2024 15:28
@cla-bot cla-bot bot added the cla:yes label Jul 12, 2024
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity1 branch from db76f4a to 30d5b6c Compare July 12, 2024 15:38
@dbt-labs dbt-labs deleted a comment from github-actions bot Jul 12, 2024
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity1 branch from 30d5b6c to 86f6109 Compare July 12, 2024 15:41
@dbt-labs dbt-labs deleted a comment from github-actions bot Jul 12, 2024
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity1.5 branch from ca0cf90 to 7ba1065 Compare July 12, 2024 15:43
# Note: ignores any granularity set on input metrics.
metric_default_time_granularity = metric.time_granularity or max(
TimeGranularity.DAY,
self._semantic_manifest_lookup.metric_lookup.get_min_queryable_time_granularity(node.metric_reference),
Copy link
Contributor

Choose a reason for hiding this comment

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

Double checking that we want the default to be the min. queryable time granularity if it's not set in the configs?

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 product decision was to default to:

  1. DAY, if available (i.e., for dimensions with DAY granularity or smaller)
  2. Min queryable granularity if DAY is not available (for dimensions with WEEK granularity or larger).
    And this should implement that behavior! So this won't change the behavior for existing dimensions - will mainly be a change once people start adding sub-daily dimensions.

Base automatically changed from court/default-granularity1 to main July 15, 2024 02:42
@courtneyholcomb courtneyholcomb force-pushed the court/default-granularity1.5 branch from 7ba1065 to 8cdb31f Compare July 15, 2024 02:43
@courtneyholcomb courtneyholcomb enabled auto-merge (squash) July 15, 2024 02:47
@courtneyholcomb courtneyholcomb merged commit 3380f5e into main Jul 15, 2024
15 checks passed
@courtneyholcomb courtneyholcomb deleted the court/default-granularity1.5 branch July 15, 2024 02:47
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