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

feat(hogql): hidden aliases for special fields #18725

Merged
merged 28 commits into from
Nov 27, 2023
Merged

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Nov 17, 2023

Problem

The following doesn't work:

select * from (SELECT timestamp FROM events)

because we don't add AS timestamp in the generated ClickHouse SQL here when wrapping with a function:

SELECT timestamp FROM (SELECT toTimeZone(events.timestamp, %(hogql_val_0)s) FROM events WHERE equals(events.team_id, 1)) -- error

This also caused issues with the custom SQL columns I was trying to implement.

Changes

Now this works.

I wrapped each field with a "hidden alias" node, which will be ignored during printing... except if it's the first child of a select column.

Alternatives

This is not the first implementation of this, nor the only way to do it. An earlier proposal (from @Twixes) was effectively to use dedicated named columnExpr nodes for select fields, and only for select fields.

I thought about it hard, but in the end decided against it. A columnExpr is effectively just an alias that can be used in one place. It brings clarity in some places, but makes other things messier. I imagine we'd have to rework many places that build select queries from AST nodes. Then it'll also make copying nodes between trees harder, as you'll have to deal with remove_column_expr style functions everywhere.

All of this would have been doable, but we got most of it for free by reusing aliases... including correct printing, ignoring and type extraction (the ast.FieldAliasType was already there and integrated into a lot of places).

I hope this is the least evil best of both world approach, but time will tell. I'd personally be happy to merge it in and get the custom SQL column expressions live already :).

How did you test this code?

Wrote tests, adapted old ones, made sure it's all green.

@mariusandra
Copy link
Collaborator Author

This breaks some tests, so will convert to draft and finish Monday. 👋

@mariusandra mariusandra marked this pull request as draft November 17, 2023 18:59
Copy link
Contributor

Size Change: 0 B

Total Size: 2.01 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 2.01 MB

compressed-size-action

@mariusandra mariusandra changed the base branch from master to resolver-snapshots November 21, 2023 21:49
@mariusandra mariusandra marked this pull request as ready for review November 22, 2023 08:54
@mariusandra
Copy link
Collaborator Author

This is ready for a look! It's blocking a) custom SQL columns, b) PoEv2 with the overrides table, c) Lifecycle insights 😅

Base automatically changed from resolver-snapshots to master November 22, 2023 10:02
Copy link
Member

@Gilbert09 Gilbert09 left a comment

Choose a reason for hiding this comment

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

Great test coverage

@mariusandra mariusandra merged commit 0582506 into master Nov 27, 2023
71 checks passed
@mariusandra mariusandra deleted the hogql-hidden-aliases branch November 27, 2023 11:20
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