feat: hogql editor pane #14688

pauldambra · 2023-03-10T19:07:04Z

Problem

We want an hogql editor pane

Changes

adds one
better hiding of data table chrome in insight view
better hiding of data table chrome in dashboard view
removes readonly props from query and query context
~~adds a hideInlineEditorButton context since on insight views the query is editable but that button should be hidden~~
adds a placement to the context since the query chrome varies in some circumstances
only show the query tab to staff users while we figure the UX out

It's not done done, but I'm going to stack on top of this since there are a few loosely related changes and it'll get to be a huge PR - see #13927

How did you test this code?

👀 locally

mariusandra · 2023-03-10T21:16:43Z

Had to check it out, even if it's draft! 👍

I noticed the second "refresh" button behind the "..." as the black tooltip alerted me to it. However we already have the other "reload" button that's part of the data table.

These data table queries can be as hard as insight queries, so I'm wondering what can be done. My take is that as something geared towards power users, I'd like to be able to reload my sql query when I choose to, or I'll just start fudging around with -- 1111 at the end of the line to make it a unique query, or beat the system some other way.

This PR makes it a bit better - we can now cancel queries if we want to abort the refresh - but ultimately I guess in "edit mode" we should hide the top level refresh from the sql table view.

mariusandra

Loving it, and found some things:

Clicking "new insight" -> "sql" doesn't load the right view
Now that the refresh button is gone, there's no way to cancel the running query. I'd still advocate for bringing it back. All removing it does is make people write stupid queries like this:

When running SQL queries on top of a data warehouse, you're usually dealing with queries that finish in several seconds to several minutes. Having some way to actually know the query running time is very very useful. I'd use it to adjust the sampling factor for example to make sure all results come back in a reasonable amount of time when I'm exploring.
Instead of the "Q", could we use the pancakes on both icons?

Could this say "SQL" instead?

Can we hide this additional "Query" tab for now.... and stick something like a <BetaTag /> behind "SQL"? I'd like data tables as insights, but let's scope it down and release one thing at a time. The UX with the "query" tab as it is now seems rather confusing...
Also left one comment inline about how to pass the context...

mariusandra · 2023-03-13T11:53:59Z

frontend/src/queries/nodes/DataTable/DataTable.tsx

+        isOnInsightPage(context) ? null : showReload ? canLoadNewData ? <AutoLoad /> : <Reload /> : null,
+        isOnInsightPage(context) ? null : showElapsedTime ? <ElapsedTime /> : null,


I think the logic is upside down here. Think of the "frontend/src/queries" folder as a self-contained library.... and everything under "frontend/src/scenes" as "the posthog app".

Other than, yes, some query nodes being called "insight trends node", the data table itself shouldn't care about the context it's being called on. It shouldn't know or care that such a thing like "an insight page" even exists.

Is there any better way to get the same meaning across? Some things to ponder:

The queries have a bunch of parameters, and similar to the /events page, you can just add showReload: false onto new insights if you want to hide just this (I'd still show it thought)... so instead of moving the "if" into the data table, just have it at the scene level.

The less different yes/no state we have, the better. We're going to have a lot of different types of query nodes, and the complexity will skyrocket if there's a "insight mode", "dashboard mode", "full query mode", "inline mode", "superuser mode", etc. If we can have just "view" and "edit", or adding one, just "inline", "view" and "edit" that'd be rather contained.

Right now, a query node with the minimum amount of params opens the "inline" view (e.g. {kind: 'DataTableNode', source: {kind:'EventsNode'}}). Adding "full:true" opens the edit view. There's no "view" mode.

Should we swap full: true to mode: 'inline' | 'view' | 'edit' or should this even be a separate parameter to the <Query /> tag, not as part of the query itself?... 🤔

Wasn't expecting #14709 to grow so much...

I was worried about the amount of showX this has and didn't want to add more. But this is just a showX but with a different name.

Happy to switch it out... we can follow up by reducing the number of things you can toggle if need be

If I'm not hiding the reload button then I don't need to pass that context in at all.

I think separately we need to make sure we can measure performance of these queries in the time-to-see data and then decide if we want to hide the reload button for the same reasons as with insights. But don't need to resolve that right now IMHO

pauldambra · 2023-03-13T12:03:04Z

Ah, the joy of language merging with Friday brain...

By "hide the top level refresh" you didn't mean the reload button 🤣

Clicking "new insight" -> "sql" doesn't load the right view

~~I think that ~~#14709~~ #14696 will fix that, but should at least get that in first and check~~

Ah, no, I was wrong on both counts. I'll get #14709 in and then I know what the fix is.

Can we hide this additional "Query" tab for now.... and stick something like a behind "SQL"?

Yep, I'm using the query tab as a debug place much like the query page... I guess the query page should move entirely to the query tab eventually. Since I use it so much I'll stick another feature flag so we can toggle things to our hearts' content

mariusandra

Had a look again, a test is failing, and found two other things to say 🙊. Also still the isOnInsightPage(context) question is an open one 🤔

frontend/src/lib/constants.tsx

frontend/src/scenes/insights/InsightNav/insightNavLogic.ts

pauldambra · 2023-03-13T17:52:56Z

@mariusandra ok, I think this is mergeable.. although needs follow-up like #14709

have set it so the query tab only shows for staff users until we know we either don't need that tab or are happy with the flow.

posthog-bot · 2023-03-13T18:12:22Z

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
webkit: 0 added, 0 modified, 0 deleted
firefox: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

posthog-bot · 2023-03-13T18:24:20Z

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
webkit: 0 added, 0 modified, 0 deleted
firefox: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

mariusandra

Looking good and let's get it in!

Found one quirk though. When I reload, click "insights" and "new" -> "sql", it becomes oddly flakey when I move away from the "sql" tab, especially while it's still loading:

It works fine when you start with "insights" -> "new" -> "something else"

mariusandra · 2023-03-13T20:06:06Z

frontend/src/queries/schema.ts

@@ -240,6 +241,7 @@ export interface DataTableNode extends Node {
    hiddenColumns?: HogQLExpression[]
    /** Show with most visual options enabled. Used in scenes. */
    full?: boolean
+


Gonna block everything for this space now. :police_hog:

I'd guess the loading weirdness is some inconsistency with coordinating filters and queries state which is getting a bit tangly now. But will take a look...

* feat: hogql editor pane * schema build * type fix * hide reload in insight placement * push the query tab for just staff users (while things are in flux) * user can be null * remove Q * put insight query tab behind a flag * fix loading from drop down: requires #14709 * less is more * quick and dirty show correct editor panel until #14709 is in * don't hide the reload button * copy url change in from #14709 to fix new insight dropdown in this PR * add beta tag to tab label * only show query tab to posthog staff * Update UI snapshots for `chromium` (2) * Update UI snapshots for `chromium` (2) * fix things missed while rushing 🙈 * slightly better viewbox * don't show SQL as an option in new insights if the feature flag isn't on * very important change --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

pauldambra added 4 commits March 10, 2023 19:05

feat: hogql editor pane

884479b

Merge branch 'master' into feat/hogql-editor-tab

f3ba09a

schema build

c6a849c

type fix

17df5c1

mariusandra mentioned this pull request Mar 10, 2023

HogQL & Data Exploration next steps PostHog/meta#81

Open

pauldambra added 3 commits March 11, 2023 09:48

hide reload in insight placement

4b0e679

Merge branch 'master' into feat/hogql-editor-tab

fd803e9

push the query tab for just staff users (while things are in flux)

07e379a

pauldambra requested a review from mariusandra March 11, 2023 13:17

pauldambra marked this pull request as ready for review March 11, 2023 13:17

pauldambra mentioned this pull request Mar 11, 2023

fix: opening queries #14695

Closed

user can be null

f9535d0

pauldambra mentioned this pull request Mar 13, 2023

feat: replace open in query builder with new insight page #14709

Merged

mariusandra reviewed Mar 13, 2023

View reviewed changes

pauldambra added 4 commits March 13, 2023 12:38

Merge branch 'master' into feat/hogql-editor-tab

5d14e59

remove Q

4f64c0f

put insight query tab behind a flag

4d47373

fix loading from drop down: requires #14709

b11a1e1

mariusandra reviewed Mar 13, 2023

View reviewed changes

frontend/src/lib/constants.tsx Outdated Show resolved Hide resolved

frontend/src/scenes/insights/InsightNav/insightNavLogic.ts Show resolved Hide resolved

pauldambra added 7 commits March 13, 2023 17:09

less is more

7263fd7

quick and dirty show correct editor panel until #14709 is in

5f6c9b8

don't hide the reload button

e0359f7

Merge branch 'master' into feat/hogql-editor-tab

c5ff1de

copy url change in from #14709 to fix new insight dropdown in this PR

2ec97bd

add beta tag to tab label

a9965ea

only show query tab to posthog staff

55e7926

pauldambra requested a review from mariusandra March 13, 2023 17:53

Update UI snapshots for chromium (2)

888b8c6

Update UI snapshots for chromium (2)

d7a1c0b

pauldambra added 2 commits March 13, 2023 19:03

fix things missed while rushing 🙈

a29b554

slightly better viewbox

f337914

mariusandra approved these changes Mar 13, 2023

View reviewed changes

pauldambra added 2 commits March 13, 2023 20:25

don't show SQL as an option in new insights if the feature flag isn't on

cd68069

very important change

cc3c135

pauldambra merged commit aadca5b into master Mar 13, 2023

pauldambra deleted the feat/hogql-editor-tab branch March 13, 2023 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: hogql editor pane #14688

feat: hogql editor pane #14688

pauldambra commented Mar 10, 2023 •

edited

Loading

mariusandra commented Mar 10, 2023

mariusandra left a comment

mariusandra Mar 13, 2023

pauldambra Mar 13, 2023

pauldambra Mar 13, 2023

pauldambra commented Mar 13, 2023 •

edited

Loading

mariusandra left a comment

pauldambra commented Mar 13, 2023

posthog-bot commented Mar 13, 2023

posthog-bot commented Mar 13, 2023

mariusandra left a comment

mariusandra Mar 13, 2023

pauldambra Mar 13, 2023

		isOnInsightPage(context) ? null : showReload ? canLoadNewData ? <AutoLoad /> : <Reload /> : null,
		isOnInsightPage(context) ? null : showElapsedTime ? <ElapsedTime /> : null,

feat: hogql editor pane #14688

feat: hogql editor pane #14688

Conversation

pauldambra commented Mar 10, 2023 • edited Loading

Problem

Changes

How did you test this code?

mariusandra commented Mar 10, 2023

mariusandra left a comment

Choose a reason for hiding this comment

mariusandra Mar 13, 2023

Choose a reason for hiding this comment

pauldambra Mar 13, 2023

Choose a reason for hiding this comment

pauldambra Mar 13, 2023

Choose a reason for hiding this comment

pauldambra commented Mar 13, 2023 • edited Loading

mariusandra left a comment

Choose a reason for hiding this comment

pauldambra commented Mar 13, 2023

posthog-bot commented Mar 13, 2023

📸 UI snapshots have been updated

posthog-bot commented Mar 13, 2023

📸 UI snapshots have been updated

mariusandra left a comment

Choose a reason for hiding this comment

mariusandra Mar 13, 2023

Choose a reason for hiding this comment

pauldambra Mar 13, 2023

Choose a reason for hiding this comment

pauldambra commented Mar 10, 2023 •

edited

Loading

pauldambra commented Mar 13, 2023 •

edited

Loading