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

Add Primary Entity Prefix When Specifying Dimensions in the WhereFilter #128

Merged
merged 2 commits into from
Jul 28, 2023

Conversation

plypaul
Copy link
Contributor

@plypaul plypaul commented Jul 27, 2023

Resolves #123

Description

With the invariant that all dimensions are associated with a primary entity, require the specification of the entity when using dimensions in the where filter.

e.g.

dimension('capacity_latest') > 10

->

dimension('listing__capacity_latest') > 10

Checklist

@cla-bot cla-bot bot added the cla:yes label Jul 27, 2023
@plypaul plypaul changed the base branch from main to plypaul--43--add-primary-entity July 27, 2023 23:55
@plypaul plypaul changed the title Support / Require Primary Entity Prefix When Specifying Dimensions in the WhereFilter Add Primary Entity Prefix When Specifying Dimensions in the WhereFilter Jul 27, 2023
@plypaul plypaul force-pushed the plypaul--44--primary-entity-in-where branch from 1dc2eb0 to 90fece9 Compare July 27, 2023 23:56
@plypaul plypaul marked this pull request as ready for review July 28, 2023 00:00
@plypaul plypaul requested a review from QMalcolm July 28, 2023 00:00
Copy link
Collaborator

@QMalcolm QMalcolm left a comment

Choose a reason for hiding this comment

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

One nit, but otherwise looks wonderful!

dbt_semantic_interfaces/naming/dundered.py Outdated Show resolved Hide resolved
dimension_reference=DimensionReference(element_name=group_by_item_name.element_name),
entity_path=(
tuple(EntityReference(element_name=arg) for arg in entity_path)
+ group_by_item_name.entity_links
Copy link
Collaborator

Choose a reason for hiding this comment

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

So the primary entity will be the last one in the entity_path tuple. Cool Cool 👍

@plypaul plypaul force-pushed the plypaul--43--add-primary-entity branch from 87b3fd4 to 49e1388 Compare July 28, 2023 18:06
Copy link
Collaborator

@tlento tlento left a comment

Choose a reason for hiding this comment

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

YAY!

I don't like time granularity dunder joins but it's better to have them here at this time. Let's just annotate it as undesirable in the docstring.

Comment on lines +19 to +21
e.g. listing__ds__week ->
entity_links: ["listing"]
element_name: "ds"
granularity: TimeGranularity.WEEK
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we indicate that including time granularity is a legacy anti-pattern and no implementer should be relying on it? It has caused so much trouble, and we are trying to eliminate it somehow or other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@plypaul plypaul force-pushed the plypaul--44--primary-entity-in-where branch from 90fece9 to 7351937 Compare July 28, 2023 18:10
Base automatically changed from plypaul--43--add-primary-entity to main July 28, 2023 18:11
@plypaul plypaul force-pushed the plypaul--44--primary-entity-in-where branch 4 times, most recently from 4aa22a6 to df0645b Compare July 28, 2023 19:12
plypaul added 2 commits July 28, 2023 12:19
With the invariant that all dimensions are associated with a primary entity,
require the specification of the entity when using dimensions in the where
filter.

e.g.

```
dimension('capacity_latest') > 10

->

dimension('listing__capacity_latest') > 10
```
@plypaul plypaul force-pushed the plypaul--44--primary-entity-in-where branch from df0645b to f322968 Compare July 28, 2023 19:19
@plypaul plypaul merged commit cc7a031 into main Jul 28, 2023
@plypaul plypaul deleted the plypaul--44--primary-entity-in-where branch July 28, 2023 19:21
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.

[Feature] Add Primary Entity Field For Local Dimensions
3 participants