fix(sqllab): disable Save dataset until the query runs successfully - #43330
fix(sqllab): disable Save dataset until the query runs successfully#43330msyavuz wants to merge 3 commits into
Conversation
The Save dataset toolbar button was gated only on whether the database allows virtual table explore, so it stayed clickable after a failed run and would build a virtual dataset from stale or empty result columns. Gate it on the latest query state, mirroring the Schedule query button.
dosubot
Bot
added
change:frontend
Code Review Agent Run #8683b6Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| } | ||
| saveQueryWarning={saveQueryWarning} | ||
| database={database} | ||
| canSaveDataset={latestQuery?.state === 'success'} |
There was a problem hiding this comment.
Suggestion: The success state belongs to the previous latestQuery, and editing SQL does not clear or replace latestQueryId, so this remains enabled after a user changes the SQL following a successful run. The dataset modal then receives the newly edited SQL together with columns from the older result. Track the SQL associated with the successful query or invalidate dataset saving whenever the editor SQL changes. [stale reference]
Severity Level: Major ⚠️
- ❌ Dataset creation can save unexecuted edited SQL.
- ⚠️ Dataset columns can come from an earlier query result.
- ⚠️ SQL Lab presents stale success state after edits.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/SqlLab/components/SqlEditor/index.tsx
**Line:** 868:868
**Comment:**
*Stale Reference: The success state belongs to the previous `latestQuery`, and editing SQL does not clear or replace `latestQueryId`, so this remains enabled after a user changes the SQL following a successful run. The dataset modal then receives the newly edited SQL together with columns from the older result. Track the SQL associated with the successful query or invalidate dataset saving whenever the editor SQL changes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
This is pre-existing and orthogonal to this PR
|
The flagged issue is correct: the current implementation only checks if the To resolve this, you should track whether the SQL in the editor matches the SQL of the successful query. A concise way to implement this is to compare the current editor SQL with the SQL of the Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset-frontend/src/SqlLab/components/SqlEditor/index.tsx |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #43330 +/- ##
=======================================
Coverage 66.67% 66.67%
=======================================
Files 2876 2876
Lines 164007 164011 +4
Branches 37834 37838 +4
=======================================
+ Hits 109347 109351 +4
Misses 52514 52514
Partials 2146 2146
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } | ||
| saveQueryWarning={saveQueryWarning} | ||
| database={database} | ||
| canSaveDataset={latestQuery?.state === 'success'} |
There was a problem hiding this comment.
Nit: This uses a raw string 'success' but the test file imports QueryState.Success from @superset-ui/core. Could we use the enum here too for consistency and type safety?
canSaveDataset={latestQuery?.state === QueryState.Success}
| color="default" | ||
| variant="text" | ||
| onClick={() => onSaveAsExplore?.()} | ||
| disabled={!canSaveDataset} |
There was a problem hiding this comment.
Question: when onSaveAsExplore is undefined (database doesn't support virtual table explore), this button isn't rendered at all, so the user never sees the "run query first" disabled state. Is that intentional? Might be worth rendering the button as disabled in that case too, so the user understands why they can't save as dataset — otherwise it silently disappears.
There was a problem hiding this comment.
Yes, allows_virtual_table_explore is a permanent database capability rather than a transient state, so a always-disabled button would be noise on those databases. The hide-vs-disable behavior there is pre-existing and untouched by this PR; happy to revisit separately if we want capability gaps surfaced in the UI.
|
What if the same query worked correctly before, but when we rerun it, the database happens to go down at that moment? Would we then have to wait for the DB to be fixed before saving the query? I don't think it's a good idea for us to take responsibility for actions that are ultimately under the user's control. |
Save query is untouched:
Or are you saying we shouldn't block not working datasets from being created. If that is the point i can reframe the condition as |
Code Review Agent Run #1ecf4fActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
EnxDev's Review Agent — #43330 · HEAD 356b916comment — Right fix, right pattern, real regression tests. But CI is green (67 passing, 0 failing); codecov reports all modified lines covered. Title is Conventional-Commits valid. 🔴 Functional
🟡 Should-fix
🙌 Praise
Note on the open threadThe DB-goes-down scenario raised in the thread doesn't cost anything today: |
A query can be in the success state with no results in the store — the post-reload hydration path keeps only the results key — so gating on the query state alone still allowed saving a dataset with no columns, which the overwrite path would write over an existing dataset's metadata.
I think we shouldn't block saving dataset at all. Even if there is query error. For example, I'm analyst and I made query in SQLab that fetched a lot of data. Superset retrieved data, but DB went down in next second after this. Analyst want to save dataset with this query but have no ability to do this even if the SQL expression is correct, 'cause of crashed DB. |
Code Review Agent Run #3f7557Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
4 participants
SUMMARY
In SQL Lab the Save dataset toolbar button was gated only on
database.allows_virtual_table_explore, never on the query result. After a failed run it stayed enabled, so you could create a virtual dataset off SQL that produced no result set (columns come fromlatestQuery.results.columns, which is stale from an earlier success or empty).The button is now gated on
latestQuery?.state === 'success'and rendered disabled with the tooltip "You must run the query successfully first" — the same pattern the Schedule query button already uses. Save query is unaffected; only dataset creation requires a successful run.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
SELECT bad FROM definitely_not_a_table_xyz.Unit tests:
npm run test -- src/SqlLab/components/SaveDatasetActionButton src/SqlLab/components/SqlEditorADDITIONAL INFORMATION