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

Remove call_parameter_sets accessors from WhereFilter and WhereFilterIntersection #348

Open
tlento opened this issue Sep 18, 2024 · 0 comments

Comments

@tlento
Copy link
Collaborator

tlento commented Sep 18, 2024

The WhereFilter interface has a property called call_parameter_sets, which essentially provides a structured way of accessing the semantic manifest elements referenced in the WhereFilter itself. The idea was that implementers of these interfaces could allow for an arbitrary filter string as input and then provide some mechanism for accessing that set of elements while guaranteeing a consistent interface for callers to access the element struct.

In practice, what we've done is split the semantic layer filter string spec - which is not specified as part of dbt-semantic-interfaces in any way - between MetricFlow and this repo. This adds a ton of complexity while also giving an implicit endorsement of whatever filter string format we happen to using at the moment.

With the addition of new element annotations like custom time granularities the problems with the current layout became more stark - we now need to be able to determine if an arbitrary string element references a custom granularity or something else. Doing this requires a particular order of operations in model parsing implementations, one which is fundamentally not enforceable from dbt-semantic-interfaces.

We hit a hard boundary on this with call_parameter_sets in #346, and in that PR we realized call_parameter_sets would now need to be parsed out of the where filter string in a secondary manifest parsing phase. The only options for enforcing this order of operations are:

  1. Require the output of the first manifest parsing phase in the call_parameter_sets accessor, which breaks the current interface
  2. Remove the call_parameter_sets accessor and effectively push the extraction of those sets to an external parsing module

We are opting for the second approach for three reasons:

  1. This accessor is not used in any of our base parsing operations, only at query compile time within MetricFlow, which means we can remove this property at any time without causing issues for the dbt core parser
  2. The external parsing component already exists - the property accessor simply invokes it
  3. By removing the method we have the option of making the new input to the parsing component optional, preserving backwards compatibility and easing any rollout of new required parameters.
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

No branches or pull requests

1 participant