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(discover) Add equation support to SnQL #29394

Merged
merged 3 commits into from
Oct 19, 2021

Conversation

wmak
Copy link
Member

@wmak wmak commented Oct 18, 2021

  • This allows the snql query builder to accept equations
  • Had to introduce some wonky importing here to avoid circular imports
  • Otherwise the idea was to pass the parsed equations to the query
    builder which can then resolve the functions and columns as needed

- This allows the snql query builder to accept equations
- Had to introduce some wonky importing here to avoid circular imports
- Otherwise the idea was to pass the parsed equations to the query
  builder which can then resolve the functions and columns as needed
@wmak wmak requested a review from a team October 18, 2021 22:47
@wmak wmak requested a review from a team as a code owner October 18, 2021 22:47
Comment on lines +347 to +349
# TODO: currently returning "resolved_equations" for the json syntax
# once we're converted to SnQL this should only return parsed_equations
parsed_equations.append(parsed_equation)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Move this below the resolved_equations.append(...) because it is now splitting apart the corresponding comment.

result = Function(equation.operator, [lhs, rhs], alias)
return result

def _resolve_equation_side(self, side: OperationSideType) -> Union[SelectType, float]:
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think this is called an operand.

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'll update in a follow up PR, cause I've used side in a buuunch of places

Comment on lines 2893 to 2894
def is_equation_column(self, column: SelectType) -> bool:
return isinstance(column, CurriedFunction) and column.alias.startswith("equation[")
Copy link
Member

Choose a reason for hiding this comment

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

If I recall correctly, equations have to contain at least 1 operator, meaning a literal is not a valid equation right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! A valid equation has to contain at least 1 operator 👍

@wmak wmak enabled auto-merge (squash) October 19, 2021 19:05
@wmak wmak merged commit d6a0926 into master Oct 19, 2021
@wmak wmak deleted the wmak/feat/snql-equation-support branch October 19, 2021 19:26
@github-actions github-actions bot locked and limited conversation to collaborators Nov 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants