Add DatePart Support to ValidLinkableSpecResolver
#911
Conversation
|
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. |
plypaul
force-pushed
the
plypaul--58.10--resolution-dag
branch
from
9ade7e7 to
affb229
Compare
plypaul
force-pushed
the
plypaul--58.11--add-date-part-to-resolver
branch
from
ad2df4e to
a2771ac
Compare
| # If no metrics are specified, the query interface supports distinct dimension values from a single semantic | ||
| # model. |
There was a problem hiding this comment.
Is this true? Because build_plan_for_distinct_values doesn't behave as if this is true. For that matter, neither does this code. Maybe we just remove this comment?
There was a problem hiding this comment.
Well dang, looks like I misread the method. I'll have to update this and a few other parts in the stakc.
There was a problem hiding this comment.
Actually, it does:
| # No date part, just the metric time at a different grain. | ||
| (None,) | ||
| # date part of a metric time at a different grain. | ||
| + tuple(date_part for date_part in DatePart if time_granularity.to_int() <= date_part.to_int()) |
There was a problem hiding this comment.
I see this granularity/date_part comparison all over the place and it's confusing every time. Maybe we should add a method to DatePart that does this and document the mechanics of the comparison there.
There was a problem hiding this comment.
I agree that we should. The reason why this was in place was that there was a bug in DSI (dbt-labs/dbt-semantic-interfaces#212), so I did this while that was getting pulled in to MF. I'll handle this as a follow up.
| possible_date_parts: Sequence[Optional[DatePart]] = ( | ||
| # No date part, just the metric time at a different grain. | ||
| (None,) | ||
| # date part of a metric time at a different grain. | ||
| + tuple(date_part for date_part in DatePart if time_granularity.to_int() <= date_part.to_int()) | ||
| ) |
There was a problem hiding this comment.
nit: I find this hard to read because it looks like a tuple of tuples and that leading + gets elided a bit by the commentary. Maybe it's a "Tom, you need reading glasses" problem, but there it is. There are lots of ways to restructure this so it's easier to see. Here's one:
# No date part (i.e., just metric time) plus all date parts for metric time at a different grain
possible_date_parts = (None,) + tuple([date_part for date_part....])
In theory the typechecker should infer that correctly but if it doesn't it's more contained and easier to reorient. Note you can also keep the multi-line commentary by using locals and just doing the assignment at the end, whatever works.
There was a problem hiding this comment.
Will update before merging.
plypaul
force-pushed
the
plypaul--58.10--resolution-dag
branch
2 times, most recently
from
a5e0bef to
8bb8e87
Compare
Adds time dimension specs with the date_part in returned results so that spec patterns can be used to resolve ambiguous group-by-items.
plypaul
force-pushed
the
plypaul--58.11--add-date-part-to-resolver
branch
from
a2771ac to
0d5afea
Compare
Description
ValidLinkableSpecResolveris used to list specs that are available for querying metrics. Since we addeddate_parttoTimeDimensionSpecs, this PR updatesValidLinkableSpecResolverto include them as well.