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): arrays and tuples #14986

Merged
merged 39 commits into from
Apr 6, 2023
Merged

feat(hogql): arrays and tuples #14986

merged 39 commits into from
Apr 6, 2023

Conversation

mariusandra
Copy link
Collaborator

Changes

Support arrays and tuples in HogQL: [] and (1, 2)

How did you test this code?

Wrote tests for the parser and printer.

@mariusandra mariusandra removed the request for review from Twixes April 5, 2023 13:51
Base automatically changed from hogql-functions to master April 6, 2023 09:38
@mariusandra mariusandra marked this pull request as ready for review April 6, 2023 13:20
Comment on lines +22 to +29
select_query = ast.SelectQuery(
select=[node], select_from=ast.JoinExpr(table=ast.Field(chain=["events"])), ref=select_query_ref
)
prepared_ast: ast.SelectQuery = cast(
ast.SelectQuery,
prepare_ast_for_printing(select_query, context=context, dialect=dialect, stack=[select_query]),
)
return print_prepared_ast(prepared_ast.select[0], context=context, dialect=dialect, stack=[select_query])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This translate_hogql function is only used to print HogQL filters inside legacy insights. There was a "undefined ref" bug that these lines fix. Basically we create a virtual "select" query, and print its first field. If the column itself got "swapped" (e.g. by adding toInt() around it), then the mutated object wouldn't get transferred into the previous virtual select. The changes above fix that --> now we prepare the entire query manually here, and then only print out the actual first select column.

Hope that makes sense 😅

Comment on lines -13 to -14
if stack:
# TODO: remove this kludge for old props
LazyTableResolver(stack=stack, context=context).visit(stack[-1])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without the previous hack in translate_hogql, we no longer need this hack 😅

Comment on lines 313 to 314
left: Expr
right: Expr
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why left and right instead of something more concrete like array and subscript?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was late and I was duplicating the exiting CompareOperation and BinaryOperation types. What you suggest makes sense. Renamed to array and property (opposed to subscript), as those were the strings I mostly used in code when passing values to left/right.

@@ -319,6 +319,19 @@ class OrderExpr(Expr):
order: Literal["ASC", "DESC"] = "ASC"


class ArrayAccess(Expr):
array: Expr
Copy link
Collaborator

Choose a reason for hiding this comment

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

Loose question: I wonder why we can't type this as Array?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sometimes arrays are returned from elsewhere... e.g arrayMap(a->a*2, [1,2,2])[1]

@mariusandra mariusandra merged commit fc50945 into master Apr 6, 2023
@mariusandra mariusandra deleted the hogql-tupleware branch April 6, 2023 15:32
fuziontech added a commit that referenced this pull request Apr 7, 2023
* master:
  fix(flags): don't enclose in overall transaction so we get latest reads (#15003)
  fix(tests): make getEventsByPerson output stable to avoid flakes (#15009)
  feat(data-exploration): convert funnel correlation to data exploration (#14963)
  fix: Set hobby deployments to 'latest' by default (#14956)
  feat(hogql): lambdas (#14987)
  feat(hogql): arrays and tuples (#14986)
  fix(funnel): always use total step count for funnel chart label (#14993)
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