feat(hogql): asterisk expander #14271
Conversation
…into hogql-symbol-resolution
mariusandra
changed the title
| if expr == "*": | ||
| expr = f'tuple({", ".join(SELECT_STAR_FROM_EVENTS_FIELDS)})' |
There was a problem hiding this comment.
The events query expects a tuple when it gets a "*" field in its select array. This will all be refactored in #14315
|
Give whoever convinced you to change the name from splotch desplotcher to asterisk expander a medal 😂 (will review in a bit!) |
|
That has to be @Twixes :D |
|
Found one more case that wasn't covered: |
|
Thanks @thmsobrmlr ! I'll merge it in, since |
| @@ -40,6 +41,8 @@ def print_ast( | |||
|
|
|||
| # modify the cloned tree as needed | |||
| if dialect == "clickhouse": | |||
| expand_asterisks(node) | |||
There was a problem hiding this comment.
you can remove the pass below now
| def get_asterisk(self) -> List[str]: | ||
| list: List[str] = [] | ||
| for field in self.__fields__.values(): | ||
| def get_asterisk(self) -> Dict[str, DatabaseField]: |
There was a problem hiding this comment.
this will need to support person on events conditionally when it's enabled 😬 (i.e. if there's a person.properties, make a decision whether to join persons table or go to the person_properties column instead).
Unclear how this should work yet, but not blocking now!
There was a problem hiding this comment.
Also food for thought: Many of our query optimisations are based on programmatically choosing which columns to select (example: choosing event.properties can OOM if it's not actually necessary for the subsequent query because these are big json payloads).
Would be nice if hogql could work with that logic, even for random client queries.
There was a problem hiding this comment.
Yeah, person properties will get their own handling. Part of it is happening in #14286 . Right now we've come sort of full circle. In that PR you can only get person properties from a joined table, not from the person_properties field :D. Since PoE is off for most everyone, I'm trying to get this working first. It should be easy to say "don't join a table, take a field instead"
There was a problem hiding this comment.
The person property "expansion" is implemented in the next PR as a separate "LazyTableResolver" step precisely so we would know and select only the fields we need. Very soon also only the properties we need.
Problem
The query
select * from anywherewould fail.Changes
*fields on tables:*would also select a few "person" fields from a virtual subtable. This "virtual subtable" doesn't work for most users on cloud (why the event explorer flag is still disabled) as most users aren't using "persons on events". Thus to keep things simple, I made the asterisk "*" only expand the actual fields on the table, excluding those person fields. I also removed the virtual "person" column/tuple that the "query_events_list" used if you selected just"person". It has also been removed in the interface for a while. All of this will be (already is) fixed in the followup PR feat(hogql): Events table based on hogql #14315How did you test this code?
Wrote tests. Checked in the browser, both by making queries and using the "old" data explorer.