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

InfluxDB: Add support for >= and <= comparison operators to IQL Query Builder #77917

Merged
merged 6 commits into from
Nov 15, 2023

Conversation

btasker
Copy link
Contributor

@btasker btasker commented Nov 9, 2023

What is this feature?

Adds support to the InfluxQL Query Editor for the >= and <= comparison operators

Why do we need this feature?

To simplify queries against numeric fields

Who is this feature for?

InfluxQL Query Builder users who wish to include WHERE conditions using greater/less than or equal to

Which issue(s) does this PR fix?:

Fixes #77916

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@btasker btasker requested a review from a team as a code owner November 9, 2023 10:30
@btasker
Copy link
Contributor Author

btasker commented Nov 9, 2023

@itsmylife this should be ready for review

@btasker
Copy link
Contributor Author

btasker commented Nov 9, 2023

Actually, this needs a front-end change too - I'll make that shortly

@btasker btasker marked this pull request as draft November 9, 2023 12:02
This ensures that the query translates correctly between raw and builder mode
@btasker btasker marked this pull request as ready for review November 9, 2023 12:58
Copy link
Contributor

@itsmylife itsmylife left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks a lot. Would you mind adding some unit tests for this?

@itsmylife itsmylife added this to the 10.3.x milestone Nov 11, 2023
@btasker
Copy link
Contributor Author

btasker commented Nov 13, 2023

Whoops, yes - added

This preserves the pre-existing behaviour, fixing a failing test
@itsmylife
Copy link
Contributor

There is a unit test failing in queryUtils. Could you please take a look at this?
Also a linting error:

src/public/app/plugins/datasource/influxdb/influx_query_model.ts
  166:78  error  Expected '!==' and instead saw '!='  eqeqeq

@@ -163,7 +163,7 @@ export default class InfluxQueryModel {
if (interpolate) {
value = this.templateSrv.replace(value, this.scopedVars);
}
if (!operator.startsWith('>') && !operator.startsWith('<') && operator != '<>') {
if ((!operator.startsWith('>') && !operator.startsWith('<')) || operator === '<>') {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep - I'd inverted the check

Copy link
Contributor

@bohandley bohandley left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@gtk-grafana gtk-grafana left a comment

Choose a reason for hiding this comment

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

Code looks good but I'm seeing a bug with the inclusivity of the comparison operators. I currently have the influxdbBackendMigration feature flag enabled
image
It looks as though the < and > operators are inclusive, but not entirely?
image

Also it looks as though the <= and >= operators aren't entirely inclusive? I would expect 2 >= 2 <= 2 === true
image

@ismail Something we might want to address (in another issue/PR), would be to smooth out the UI in the query builder with custom input.
image.
When you're searching against a numeric field we should have better support for custom input, instead of always seeing the "No options found" even if you'd already inserted a custom input.

Another nit that should be addressed in another issue is always getting <> as the first suggestion when you type < or > is annoying. It should be at the bottom, or not included in the dropdown as a suggestion as we already have !=
image

Copy link
Contributor

@gtk-grafana gtk-grafana left a comment

Choose a reason for hiding this comment

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

Actually nevermind! This is already a "problem" on main, so I'm actually fine with merging this in as is. This is a problem with floats, and the rounding of them in the viz. If I compare against the actual values returned by influx, and not what is visualized, this works as expected!

@btasker
Copy link
Contributor Author

btasker commented Nov 14, 2023

@gtk-grafana in your second condition you're comparing against a different field (usage_guest vs usage_idle), is that deliberate?

@gtk-grafana
Copy link
Contributor

@btasker no that was a mistake, and I had also left an aggregation in my query which was giving me slighly different values each query 🤦 . After checking against values that aren't changing each query, it works as expected.
Sorry about the false alarm, and thanks again for the great work!

@ismail
Copy link

ismail commented Nov 14, 2023

@gtk-grafana You want @itsmylife ;)

Copy link
Contributor

@itsmylife itsmylife left a comment

Choose a reason for hiding this comment

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

🚀

@itsmylife itsmylife merged commit c06debe into grafana:main Nov 15, 2023
14 checks passed
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InfluxDB: Missing greater/less than or equal to operators (<=,>=)
6 participants