feat(data-warehouse): new SQL layout #25686
Conversation
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
|
Size Change: +45 B (0%) Total Size: 1.15 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated6 snapshot changes in total. 0 added, 6 modified, 0 deleted:
Triggered by this commit. |
There was a problem hiding this comment.
Feel free to merge this to get it out, but, I really think we should address my large comment about editor logic and building better support for a multi-tabbed editor disconnected from the query object - we should get that right before building new features IMO, but happy to discuss this in standup today
| @@ -173,6 +173,7 @@ export const urls = { | |||
| dataWarehouse: (query?: string | Record<string, any>): string => | |||
| combineUrl(`/data-warehouse`, {}, query ? { q: typeof query === 'string' ? query : JSON.stringify(query) } : {}) | |||
| .url, | |||
| sqlEditor: (): string => `/sql`, | |||
There was a problem hiding this comment.
Do we want this to be /sql-editor? Not that fussed either way, but it may be a bit clearer. Also, are we fine with using SQL and not HogQL everywhere? I think yes, but wanted to mention it
There was a problem hiding this comment.
temporary path just for testing purposes
| if (monaco) { | ||
| const uri = monaco.Uri.parse(model.path) | ||
| const newModel = monaco.editor.createModel(model.query, 'hogQL', uri) | ||
| editor?.setModel(newModel) |
There was a problem hiding this comment.
So the last saved model will be the one selected in the UI? May be good to break this out of this forEach loop
| onPressCmdEnter: (value, selectionType) => { | ||
| if (value && selectionType === 'selection') { | ||
| saveQuery(value) | ||
| } else { | ||
| saveQuery() | ||
| } | ||
| }, |
There was a problem hiding this comment.
I know this is the existing logic right now, but I wanna get away from this model of being able to "save" a selection of the query from the editor - this sets the inner setQuery of hogQLQueryEditorLogic.
Ultimately, I'd like a pattern here where the HogQL query is separated from the editor itself. We have a bunch of workarounds and less-than-ideal UX due to how the editor logic works in conjunction with dataNodeLogic, and this is a chance to completely rewrite that. An example of this is: if you update the text in the hogql editor and don't run it but save it - the query text of the last run is whats saved and not whats currently in your editor = bad and weird UX.
I'd consider thinking about writing a new logic for managing all of this model state along with the appropriate components - the current set up really isn't designed with tabs in mind. We can fix this now before its too late and becomes a tough situation later
There was a problem hiding this comment.
if you update the text in the hogql editor and don't run it but save it - the query text of the last run is whats saved and not whats currently in your editor = bad and weird UX.
This actually isn't an issue with how I used datanodelogic right now. DataNodeLogic right now is mainly used to just be fed a query and return some results. The actual query that's handled by the saving vs the editing is decoupled with a component state var setActiveQueryInput.
But, overall I do agree this logic needs some rethinking. I worked this in for now to get an idea of dependencies of the current editor but definitely open to making a new logic as there's a good bit of logic that's sitting inside the component/onMount handler
Problem
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
How did you test this code?