-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(hogql): automatic event and person property types #14795
Conversation
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.
some thinking out loud but this does what is says on the tin
event_property_values = ( | ||
PropertyDefinition.objects.filter( | ||
name__in=property_finder.event_properties, | ||
team_id=context.team_id, | ||
type__in=[None, PropertyDefinition.Type.EVENT], | ||
).values_list("name", "property_type") | ||
if property_finder.event_properties | ||
else [] | ||
) | ||
event_properties = {name: property_type for name, property_type in event_property_values if property_type} | ||
|
||
person_property_values = ( | ||
PropertyDefinition.objects.filter( | ||
name__in=property_finder.person_properties, | ||
team_id=context.team_id, | ||
type=PropertyDefinition.Type.PERSON, | ||
).values_list("name", "property_type") | ||
if property_finder.person_properties | ||
else [] | ||
) | ||
person_properties = {name: property_type for name, property_type in person_property_values if property_type} |
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.
Might be quicker for me to ask than figure it out #Lazy
How often do we call this?
Might be early-optimizing but the property types change infrequently so we can cache seen properties relatively aggressively. Makes me wonder how well our tracing covers this code 🤔
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.
This will just be called once per query in most cases. In case of non-hogql insights, it'll be called once per filter. This is where caching could make the biggest difference, but I didn't feel like overoptimising yet.
Problem
All properties were always returned as strings
Changes
Not any more! We now wrap all columns specified as
Numeric
withtoFloat64OrNull
, allDateTime
colums withparseDateTimeBestEffort
and allBoolean
columns withcol = 'true
'I think we should split
Numeric
up intoInteger
andFloat
for accuracy, as treating everything as a float is weird... and ClickHouse'sDecimal
requires a fixed precision which seems like too big of a decision to make.How did you test this code?
Wrote tests. Checked in the UI. Checking CI now for all the extra queries this makes.