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

fix: selecting from cohort people #17610

Merged
merged 7 commits into from
Sep 26, 2023
Merged

fix: selecting from cohort people #17610

merged 7 commits into from
Sep 26, 2023

Conversation

pauldambra
Copy link
Member

I don't think you can select from cohort_people at all with HogQL

Before

Screenshot 2023-09-25 at 20 25 44

Well, now you can!

After

Screenshot 2023-09-25 at 20 26 18

@pauldambra pauldambra requested a review from a team September 26, 2023 07:38
requested_fields = {"person_id": ["person_id"], "cohort_id": ["cohort_id"], **requested_fields}

fields: List[ast.Expr] = [
ast.Alias(alias=name, expr=ast.Field(chain=[table_name] + chain)) for name, chain in requested_fields.items()
]
fields: List[ast.Expr] = [ast.Field(chain=[table_name] + chain) for name, chain in requested_fields.items()]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line was there so that we always request the person and cohort fields. Without them, some queries and/or joins will just fail.

Thus, if you now select just the person_id, or the cohort_id or just something else that's not those two, will this fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to work the same with those two fields always bolted back on... e.g. select team_id from cohort_people works either way.

Is there an obvious test case to try?

Copy link
Member Author

Choose a reason for hiding this comment

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

(I've put the line ensuring they're present back in)

Copy link
Collaborator

@mariusandra mariusandra Sep 26, 2023

Choose a reason for hiding this comment

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

Hmm... actually I'll take that back. The issue I was remembering is valid only in the case of automatic joins (e.g. events.person.bla). The join constraint would fail if we didn't always select the fields we're going to use in the constraint. However... that's not relevant here, so insert homer bush gif here 🤣

Probably good/ok to select them anyway, so nothing to change here now... 🤷

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

This is good. I'm not sure why the table had all the columns as aliases in the first place. None of the others do 🤷

@pauldambra pauldambra merged commit ec4faf2 into master Sep 26, 2023
72 checks passed
@pauldambra pauldambra deleted the fix/cohort-people-select branch September 26, 2023 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants