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

[telemetryquerylanguage] add AND, OR, and parens to WHERE clauses #12213

Merged
merged 18 commits into from
Jul 14, 2022

Conversation

kentquirk
Copy link
Member

@kentquirk kentquirk commented Jul 10, 2022

Implements more complex expressions for WHERE clauses in the grammar.

Description:
The transform processor currently accepts a WHERE clause with a single condition, but there are times where multiple conditions are needed in order to filter to the correct data.

This updates the grammar to allow AND and OR with proper precedence, as well as grouping with parentheses.

Note that during development, the parser was restructured to become a standalone component usable by multiple processors, so the code was ported from #11880 (where there were many individual commits).

Link to tracking Issue:
Fixes #10195.
Closes #11880 and supercedes it.

Testing:

  • The lexer builder was extracted to a separate function
  • Tests were added for the lexer to ensure that keywords are not confused with other identifiers
  • Failing tests were added for the parser with complex conditions
  • New types added to the grammar (Expression, etc) to support more elaborate expressions
  • Grammar structured to handle precedence correctly
  • Expression evaluation will be extracted to a more general structure
  • More lexer tests
  • Parser tests to confirm proper operation
  • Add tests for boolean expression evaluators

Documentation:

  • Grammar extensions documented

@kentquirk kentquirk mentioned this pull request Jul 10, 2022
9 tasks
@kentquirk kentquirk marked this pull request as ready for review July 10, 2022 21:23
@kentquirk kentquirk requested a review from a team July 10, 2022 21:23
@kentquirk kentquirk requested a review from TylerHelmuth as a code owner July 10, 2022 21:23
@kentquirk kentquirk changed the title Add AND, OR, and parens to WHERE clauses. [telemetryquerylanguage] add AND, OR, and parens to WHERE clauses Jul 11, 2022
@TylerHelmuth
Copy link
Member

@kentquirk I am going to take an in-depth look at this today but based on my first pass I wanted to say great job. This is a complicated feature and you've modeled it in an understandable way

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

@kentquirk reviewed in depth. Mostly just nits.

pkg/telemetryquerylanguage/tql/README.md Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/parser.go Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/parser.go Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/parser.go Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/condition.go Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/boolean_value.go Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/boolean_value.go Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/boolean_value.go Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/parser.go Show resolved Hide resolved
@TylerHelmuth
Copy link
Member

/cc @anuraaga

@kentquirk
Copy link
Member Author

kentquirk commented Jul 12, 2022

@TylerHelmuth I believe I've addressed all the issues. Thanks for the fantastic feedback.

Also, I believe that I understand from slack that @anuraaga is unavailable right now.

pkg/telemetryquerylanguage/tql/parser.go Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/boolean_value.go Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/parser.go Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/boolean_value.go Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/lexer_test.go Outdated Show resolved Hide resolved
pkg/telemetryquerylanguage/tql/parser_test.go Outdated Show resolved Hide resolved
Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

One more small documentation nit, otherwise looks great.

pkg/telemetryquerylanguage/tql/README.md Outdated Show resolved Hide resolved
Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

Looks good 👍🏻

@kentquirk
Copy link
Member Author

I've fixed the nit that the linter was complaining about, and ran the global make gotidy, so hopefully this time it'll pass all the checks.

@kentquirk
Copy link
Member Author

Or not. This time i tried running make -j2 golint GROUP=other locally, and fixed things until it passed.

kentquirk and others added 3 commits July 13, 2022 14:39
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
@kentquirk
Copy link
Member Author

Rebased after go mod conflict resolution.

@codeboten codeboten merged commit 341e714 into open-telemetry:main Jul 14, 2022
@kentquirk kentquirk deleted the kent.ands-ors-2 branch July 14, 2022 17:33
@EddieEldridge
Copy link
Contributor

Just want to say thanks for doing this! I badly need to use AND and OR in my queries and this will be a great help.

@TylerHelmuth
Copy link
Member

@EddieEldridge as a heads up, this was added to the TQL package, which the transform processor doesn't depend on yet, but I have a PR open to change this.

Hopefully the PR is merged before the next release. If it isn't, then it'll be another two weeks before the processor has this functionality.

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.

[processor/transform] Add AND and OR capability to WHERE condition
6 participants