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

fix: query builder groups on columns, not tag values #16992

Merged
merged 2 commits into from
Feb 25, 2020

Conversation

hoorayimhelping
Copy link
Contributor

@hoorayimhelping hoorayimhelping commented Feb 24, 2020

Closes #16783

A picture is worth a thousand words, so here:

WRONG:
Screen Shot 2020-02-24 at 2 43 26 PM

Right:
Screen Shot 2020-02-24 at 2 42 52 PM

  • Wrong: The grouping options map to tag values
  • Right: The grouping options map to columns

@hoorayimhelping hoorayimhelping changed the title Bs make query builder actually usable fix: query builder groups on columns, not tag values Feb 24, 2020
@hoorayimhelping hoorayimhelping force-pushed the bs_make_query_builder_actually_usable branch from 55bd7d0 to 98750c4 Compare February 24, 2020 22:52
...activeQueryTags[2].values,
])
})
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this test was testing the wrong thing

Copy link
Contributor

Choose a reason for hiding this comment

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

can we test the right thing?

Copy link
Contributor Author

@hoorayimhelping hoorayimhelping Feb 25, 2020

Choose a reason for hiding this comment

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

@hoorayimhelping hoorayimhelping force-pushed the bs_make_query_builder_actually_usable branch from 98750c4 to 25b8387 Compare February 24, 2020 23:15
@@ -49,6 +48,9 @@ import {

const SEARCH_DEBOUNCE_MS = 500

// We don't show these columns in results but they're able to be grouped on
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not always true. There are scenarios where a _start and _stop are not not columns in a query response.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Russ mentioned that, but it seems like in the context of query builder, it's safe to assume we can group on those columns? I'll change the comment

...activeQueryTags[2].values,
])
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

can we test the right thing?

@hoorayimhelping hoorayimhelping force-pushed the bs_make_query_builder_actually_usable branch 2 times, most recently from 8fbf745 to 3a31f0a Compare February 25, 2020 17:41
@hoorayimhelping hoorayimhelping force-pushed the bs_make_query_builder_actually_usable branch from 3a31f0a to 215962a Compare February 25, 2020 19:24
@hoorayimhelping hoorayimhelping merged commit e6cf208 into master Feb 25, 2020
@hoorayimhelping hoorayimhelping deleted the bs_make_query_builder_actually_usable branch February 25, 2020 19:37
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.

Group By in the query builder does not show column names
2 participants