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): bunch of improvements (HogVM part 1) #16274

Merged
merged 10 commits into from
Jun 28, 2023
Merged

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Jun 27, 2023

Problem

The HogQL local evaluation PR (aka HogVM) is getting out of control. I'll split it into at least three. This is part one. It does a bit of cleanup, and adds better regex support.

Changes

  • Rename BinaryOperation to ArithmeticOperation
  • Standardise LtE to LtEq, and LE to LT_EQ
  • Update the database JSON snapshot, and make CI save it going forward
  • Instead of not(in(...)) in the printer, use shorthands like notIn() when possible
  • Adds support for regex operator ~, =~ and !~.
  • Also adds support for case insensitive regex operators ~*, =~* and !~*. We need this separately for local evaluation. ClickHouse uses re2 for regex matching, whereas Python and JS use pcre. Adding our own re2 bindings to the HogVM implementations is possible (~200kb of WASM?), but likely not worth it when we want to keep the bundle size down. E.g. for client side feature flags. A lot of the syntax between both implementations is similar, however a big difference is in how you pass flags. re2 uses (?i) inside the regex, whereas pcre uses a separate modifiers list (like /a/i). Introducing these operators seemed to be the only way to reliably do case-insensitive regex matching both locally and in ClickHouse.
  • Removes the cohort(1) function, and instead only support the id in cohort 1 syntax. We're effectively moving from in(id, cohort(1)) to inCohort(id, 1). Nothing really changes for users, this makes cohort matching easier to implement in HogVM, and unlocks further optimisations.

How did you test this code?

Added or updated tests where relevant

Copy link
Contributor

@thmsobrmlr thmsobrmlr left a comment

Choose a reason for hiding this comment

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

LGTM

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