-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(hogql): property array access #15965
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
Conversation
@PostHog I love subscribing to issues and know that a feature we need is going to be implemented <3 @jannesblobel subscribe to this issue. When this issue is complete we can build insights to derive what plugins are used. |
Haha, great to hear! There are a few more complex cases I still need to get working before it's ready. Unlikely this will be done, reviewed and merged today, but soon 👀 |
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.
LGTM
Problem
You can't access array elements in properties
Changes
Makes the following cases work:
properties.string
properties.array_str.1
properties.array_str[1]
properties.obj_array.id.1
properties.obj_array.id[1]
properties.array_array_str.1.1
properties.array_array_str[1][1]
properties.array_obj_array.1.id.1
properties.array_obj_array[1]['id'][1]
properties.array_obj_array_obj.1.id.1.id
properties.array_obj_array_obj[1].id[1].id
properties.array_obj_array_obj[1]['id'][1]['id']
properties.array_obj.1.id
properties.array_obj[1].id
Sadly for all of us, SQL indexes start from
1
. This is bound to cause confusion, so I added a helpful error if you use0
.SQL indexes can also be negative, so we can't automatically
+1
in the HogQL layer either 🙃How did you test this code?
Plenty of test changes. Tried it in the UI as well.
Out of scope
We could fork ClickHouse