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: raise error in sqllab when using reserved column name #9859

Merged
merged 1 commit into from Jun 10, 2020

Conversation

villebro
Copy link
Member

@villebro villebro commented May 20, 2020

SUMMARY

Currently SQL Lab accepts the column name __timestamp, which causes problems when pressing the "Explore" button. This usually happens when clicking the "Run in SQL Lab" button on a timeseries chart. This adds a check for column names starting with double underscores.

BEFORE

image
image

AFTER

image

TEST PLAN

Local testing + new tests

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@eugeniamz
Copy link

It is a good fix to improve the error message but CHARTs generate the query with reserved words so if the workflow is

Chart -> SQL Lab : you get the reserved word and needs to be changed if you want to Explore the data again.

If in CHARTs we don't generate reserved words, that should resolve the issue to the root.

@codecov-commenter
Copy link

codecov-commenter commented May 20, 2020

Codecov Report

Merging #9859 into master will increase coverage by 6.51%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9859      +/-   ##
==========================================
+ Coverage   64.61%   71.13%   +6.51%     
==========================================
  Files         536      588      +52     
  Lines       29405    30793    +1388     
  Branches     2879     3238     +359     
==========================================
+ Hits        19001    21904    +2903     
+ Misses      10225     8778    -1447     
+ Partials      179      111      -68     
Flag Coverage Δ
#cypress 53.95% <0.00%> (-0.09%) ⬇️
#javascript 59.30% <100.00%> (?)
#python 71.27% <ø> (ø)
Impacted Files Coverage Δ
...end/src/SqlLab/components/ExploreResultsButton.jsx 74.07% <100.00%> (+69.07%) ⬆️
superset-frontend/src/setup/setupPlugins.ts 13.33% <0.00%> (-86.67%) ⬇️
...rset-frontend/src/setup/setupErrorMessagesExtra.ts 50.00% <0.00%> (-50.00%) ⬇️
superset-frontend/src/setup/setupErrorMessages.ts 66.66% <0.00%> (-33.34%) ⬇️
.../src/components/Select/WindowedSelect/windowed.tsx 94.11% <0.00%> (-5.89%) ⬇️
superset-frontend/src/setup/setupApp.ts 25.00% <0.00%> (-3.58%) ⬇️
superset-frontend/src/featureFlags.ts 100.00% <0.00%> (ø)
superset-frontend/src/components/styles.ts 100.00% <0.00%> (ø)
superset-frontend/src/SqlLab/utils/sqlKeywords.ts 100.00% <0.00%> (ø)
...et-frontend/src/dashboard/util/isDashboardEmpty.ts 100.00% <0.00%> (ø)
... and 192 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a775e2...75e9615. Read the comment docs.

@villebro
Copy link
Member Author

@eugeniamz the thing here is that the query as such is not really usable for re-exploration, as it has time grains, filters etc burned into the query. My interpretation is that this feature is used for throwaway analysis purposes only. Perhaps we should also add a comment above the query to that effect to explain that the query is only a reflection of a certain chart, and should usually not be used as a basis for a new chart?

@eugeniamz
Copy link

I understand that this the best solution for now, at list this will avoid the back and forward. Thanks Ville!

@villebro villebro merged commit 56397d7 into apache:master Jun 10, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 0.37.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants