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

Simplify nodes that join to time spine #1535

Closed
wants to merge 4 commits into from
Closed

Conversation

courtneyholcomb
Copy link
Contributor

No description provided.

@cla-bot cla-bot bot added the cla:yes label Nov 20, 2024
Copy link

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@@ -645,15 +643,17 @@ def _build_derived_metric_output_node(

# For ratio / derived metrics with time offset, apply offset & where constraint after metric computation.
if metric_spec.has_time_offset:
queried_agg_time_dimension_specs = queried_linkable_specs.included_agg_time_dimension_specs_for_metric(
required_agg_time_dimension_specs = queried_linkable_specs.included_agg_time_dimension_specs_for_metric(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should revert this

), "Joining to time spine requires querying with metric_time or the appropriate agg_time_dimension."
output_node = JoinToTimeSpineNode.create(
parent_node=output_node,
requested_agg_time_dimension_specs=queried_agg_time_dimension_specs,
requested_agg_time_dimension_specs=[
spec.with_base_grain() for spec in required_agg_time_dimension_specs
Copy link
Contributor Author

@courtneyholcomb courtneyholcomb Nov 20, 2024

Choose a reason for hiding this comment

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

revert

@@ -1625,7 +1625,11 @@ def _build_aggregated_measure_from_measure_source_node(
# in join rendering
unaggregated_measure_node = JoinToTimeSpineNode.create(
parent_node=unaggregated_measure_node,
requested_agg_time_dimension_specs=queried_agg_time_dimension_specs,
requested_agg_time_dimension_specs=[
Copy link
Contributor Author

Choose a reason for hiding this comment

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

revert

@@ -1592,10 +1592,14 @@ def _build_aggregated_measure_from_measure_source_node(
queried_agg_time_dimension_specs = queried_linkable_specs.included_agg_time_dimension_specs_for_measure(
measure_reference=measure_spec.reference, semantic_model_lookup=self._semantic_model_lookup
)
required_agg_time_dimension_specs = required_linkable_specs.included_agg_time_dimension_specs_for_measure(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

revert


# If a cumulative metric is queried with metric_time / agg_time_dimension, join over time range.
# Otherwise, the measure will be aggregated over all time.
unaggregated_measure_node: DataflowPlanNode = measure_recipe.source_node
# TODO: can we use required here, too?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

revert

@@ -1697,7 +1701,7 @@ def _build_aggregated_measure_from_measure_source_node(
# like JoinToCustomGranularityNode, WhereConstraintNode, etc.
output_node: DataflowPlanNode = JoinToTimeSpineNode.create(
parent_node=aggregate_measures_node,
requested_agg_time_dimension_specs=queried_agg_time_dimension_specs,
requested_agg_time_dimension_specs=required_agg_time_dimension_specs,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

revert

@courtneyholcomb courtneyholcomb force-pushed the court/simplify branch 2 times, most recently from 705fe01 to 00fb89e Compare November 20, 2024 16:39
Clean up. This was causing me some confusion because we recently
introduced the concept of default grain for metrics, which is defined in
 the YAML spec, but this code was not referring to that.
There should be no functional changes in this commit, only cleanup and readability improvements. Mostly involves moving complex logic to helper functions.
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.

1 participant