Skip to content

Conversation

@wiedld
Copy link
Contributor

@wiedld wiedld commented Jan 18, 2023

Part of #6482
Closes #6496
Closes #6503

Add ability to groupby tagKeys (a.k.a. into dataSeries) for SQL queries being graphed.

Qx contract:

  • how many queries (network calls) are run:
    • when I change by query viewOptions => re-run the query
    • if I change queryText => have no viewOptions => only 1 query with run (see network tab in vid)
    • if I change queryText => have viewOptions => 2 queries will run (one for table, one for graph)
  • how are the groupby options chosen?
    • when produce results => use the table column headers for the groupby options
      • THIS IS THE SAME CONTRACT as displaying the table data. You have to re-run the query to produce results.
    • set the default groupby, to be the tagKeys
      • once we choose the bucket & measurement => we can then know the tagKeys for the default

Viz evidence: How many queries?

Screen.Recording.2023-01-17.at.6.47.56.PM.mov

Viz Evidence: How groupby options chosen?

Screen.Recording.2023-01-18.at.1.36.20.PM.mov

Checklist

  • A PR description, regardless of the triviality of this change, that communicates the value of this PR
  • Well-formatted conventional commit messages that provide context into the change
  • Documentation updated or issue created (provide link to issue/PR)
  • Signed CLA (if not already signed)
  • Feature flagged, if applicable

@wiedld wiedld changed the base branch from master to 6282/make-sql-composition-e2es January 18, 2023 03:05
@wiedld wiedld changed the title feta(6496): SQL graphing viewOptions: groupby tag keys feat(6496): SQL graphing viewOptions: groupby tag keys Jan 18, 2023
@wiedld wiedld force-pushed the 6282/make-sql-composition-e2es branch from 7393f54 to c8fa90f Compare January 18, 2023 17:35
@wiedld wiedld force-pushed the 6496/groupby-tagKeys branch from 8a1594a to fcbf912 Compare January 18, 2023 17:42
Base automatically changed from 6282/make-sql-composition-e2es to master January 18, 2023 20:10
@wiedld wiedld marked this pull request as ready for review January 18, 2023 21:45
@wiedld wiedld requested a review from a team as a code owner January 18, 2023 21:45
@wiedld wiedld requested review from a team and abshierjoel January 18, 2023 21:45
Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

A couple of minor things, but all-in-all it looks good

Comment on lines 73 to 75
const setViewOptionsAll = useCallback(
(updatedOptions: Partial<ViewOptions>) => {
saveViewOptionsAll({...viewOptionsAll, ...updatedOptions})
Copy link
Contributor

Choose a reason for hiding this comment

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

setViewsOptionAll and saveViewOptionsAll are really similarly named, and it takes a little bit of digging to understand the difference between the two. I can't think of any conventions around using set vs save - they're pretty interchangeable, so to avoid confusion, it might be worth calling this method saveViewOptionsAllMemoized to know that the only difference is this is memoized on viewOptionsAll

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was going for set == "add to setting", and save == "save to local persistance". 🤷🏼

'dataExplorer.resultsOptions.default',
DEFAULT_VIEW_OPTIONS
)
const setDefaultViewOptions = useCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

if you choose to name setViewOptionsAll as something like saveViewOptionsAllMemoized, this should also be updated to follow the same convention

Copy link
Contributor Author

@wiedld wiedld Jan 18, 2023

Choose a reason for hiding this comment

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

I'm hesitant to use the word memoized, as the definition is "to remember" -- and the ALL options is reset rather frequently (with every parent query run). Depending on the user flow, they could be memoizing the ALL (versus the DEFAULT) more or less often.

Will muse on another naming convention.

Copy link
Contributor Author

@wiedld wiedld Jan 18, 2023

Choose a reason for hiding this comment

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

See if this last commit added, helps to clarify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, here is some manual QA on the behavior for clear().
(Note: we really need to put in tests sometime soon-ish.)

Screen.Recording.2023-01-18.at.4.06.17.PM.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, works for me! Thanks for thinking about this critically!

@wiedld wiedld force-pushed the 6496/groupby-tagKeys branch from 84f4a8b to ce788ab Compare January 19, 2023 00:08
Copy link
Contributor

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

Shipit!

@wiedld wiedld added this pull request to the merge queue Jan 19, 2023
Copy link
Contributor

@abshierjoel abshierjoel left a comment

Choose a reason for hiding this comment

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

⚡🎸

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 19, 2023
@wiedld wiedld added this pull request to the merge queue Jan 19, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 19, 2023
@wiedld wiedld added this pull request to the merge queue Jan 19, 2023
Merged via the queue into master with commit 56e5af7 Jan 19, 2023
@wiedld wiedld deleted the 6496/groupby-tagKeys branch January 19, 2023 17:35
mavarius pushed a commit that referenced this pull request Jan 20, 2023
* feat(6496): viewOptions to modify query by groupby tagKeys

* fix(6496): Cannot run subquery if no bucket is selected. Show spinner when graph subquery is loading.

* feat(6496): have groupby options be a based upon returned results, with the default groupby determined via the tagKeys in the schema

* feat(6482): when choosing graph --> auto-open the Customize options in right hand panel.

* chore(6496): use better naming conventions

* feat(6496): make explicit what is persisted, and determine when it's cleared
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.

Graph customize options do not update with changes in schema selection. Add groupby tagKeys to the SQL editor.

4 participants