-
Notifications
You must be signed in to change notification settings - Fork 35
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 SQL validator support for measures #693
base: master
Are you sure you want to change the base?
Conversation
Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few high-level questions:
-
Aside from allowing measures to flow through into the LookML tree, is there anything else special happening to test them?
-
I'm not seeing any tests specifically for measures, just the refactored existing tests.
-
Should we expect performance to be impacted by including aggregates in the Explore-level queries?
@@ -120,7 +120,7 @@ def __init__( | |||
metadata = { | |||
"line_number": line_number, | |||
"lookml_url": lookml_url, | |||
"dimension": field_name, | |||
"field": field_name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noting that this is basically a breaking change as far as the API is concerned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we would need to release this with a corresponding change in the app/UI. It will likely need to account for both dimension and field, given old runs will have the old key.
@@ -54,7 +53,7 @@ def __repr__(self): | |||
) | |||
|
|||
def __eq__(self, other): | |||
if not isinstance(other, Dimension): | |||
if not isinstance(other, LookMlField): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a dimension LookMlField
and measure LookMlField
are compared, how are they marked as different? Is it the type
attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, type should do it in most cases, but your question has made me realise that technically a dimension and a measure could both be of type number
and have the same name. It's a very narrow case, but could happen.
There is a boolean measure
field on the fields JSON payload, so we could easily add that to both the model and the eq function. Does that work for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that works!
|
c605e59
to
69081ef
Compare
As far as measure-specific tests go, we may want to make sure all measure types are supported, since we don't have all of them defined on our |
Co-authored-by: Josh Temple <[email protected]>
0d2852b
to
21091a4
Compare
Change description
Adding support for measures in the SQL validator
Type of change
Related issues
Closes #368
Checklists
Security
Code review