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(explore): Fix datasource switch for table chart #21544

Merged
merged 1 commit into from
Sep 26, 2022

Conversation

codyml
Copy link
Member

@codyml codyml commented Sep 21, 2022

SUMMARY

Another casualty of #21315 was datasource switching for table chart: after that PR, switching between datasources wouldn't preserve any columns. As a side-effect, saving charts based on SQL queries instead of datasets would fail, as the save process involves switching from the query datasource to the newly-created dataset. This PR fixes the issue, which was specifically caused by #21315 turning chartState.options from an object into an array (see #21484).

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

localhost_9000_explore__form_data_key=O3JVAVF3-mFhHkTJ8F3xoCyHzxNRoottQ-ZJcNMIx1KOzPpJ8ueK6R88J-zJyptf datasource_id=1 datasource_type=query

After:

localhost_9000_explore__form_data_key=gHVMJ2BnjkTUap0aWTVz-ocYbMr5xFS3xE1sV1c8x1gdIsAPqErF4pzNNsWhFqz3 datasource_id=1 datasource_type=query

TESTING INSTRUCTIONS

  • Create a Table chart from a dataset in Raw mode and try switching datasets. Ensure that columns that exist on both datasets are preserved.
  • Create a Table chart in Raw mode from a query and try saving it. Ensure that all control values remain unchanged after save.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Sep 21, 2022

Codecov Report

Merging #21544 (f754013) into master (c66205f) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master   #21544   +/-   ##
=======================================
  Coverage   66.68%   66.68%           
=======================================
  Files        1793     1793           
  Lines       68538    68537    -1     
  Branches     7282     7282           
=======================================
  Hits        45702    45702           
+ Misses      20974    20973    -1     
  Partials     1862     1862           
Flag Coverage Δ
javascript 52.86% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...lUtils/getControlValuesCompatibleWithDatasource.ts 8.00% <0.00%> (+0.30%) ⬆️
...d/src/SqlLab/components/SaveDatasetModal/index.tsx 52.87% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@lyndsiWilliams
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@lyndsiWilliams Ephemeral environment spinning up at http://54.201.68.238:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@lyndsiWilliams lyndsiWilliams added the need:qa-review Requires QA review label Sep 21, 2022
@jinghua-qa
Copy link
Member

I found an issue that, when create chart with a result have time column, it seems like the time column will become number in explore, and user can not create time-series related charts
master:
Screen Shot 2022-09-21 at 4 36 16 PM
in this pr:
Screen Shot 2022-09-21 at 4 39 23 PM

@mayurnewase
Copy link
Contributor

fixes: #21531

@codyml
Copy link
Member Author

codyml commented Sep 22, 2022

I found an issue that, when create chart with a result have time column, it seems like the time column will become number in explore, and user can not create time-series related charts
master:
Screen Shot 2022-09-21 at 4 36 16 PM
in this pr:
Screen Shot 2022-09-21 at 4 39 23 PM

@jinghua-qa Looking into this now, thanks!

@jinghua-qa
Copy link
Member

I am able to reproduce in master for this issue, i try another dataset "Video Game Sales', when use chart-power-query, year open as # in explore and if open the dataset directly, year is a time column in the dataset. So i think this issue is not related to this PR and i will file issue seperately
Screen Shot 2022-09-23 at 12 48 53 AM
Screen Shot 2022-09-23 at 12 48 32 AM

@jinghua-qa
Copy link
Member

However when i deployed pr changes to the preset workspace and test, i saw error msg for chart save ( but the chart is actually saved)

Screen Shot 2022-09-23 at 1 08 22 AM

Copy link
Member

@stephenLYZ stephenLYZ left a comment

Choose a reason for hiding this comment

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

Thanks for the fix. It would be nice if we can add some tests.

@jinghua-qa
Copy link
Member

Have tested in a workspace with recent superset master and it is working fine, the issue for the time column because string is not related to this PR. LGTM

@codyml
Copy link
Member Author

codyml commented Sep 26, 2022

Thanks for the fix. It would be nice if we can add some tests.

@stephenLYZ Thanks for the review! I have adding tests for this case on my list, am going to tackle it separately.

@jinghua-qa jinghua-qa merged commit 954fc89 into apache:master Sep 26, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 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 need:qa-review Requires QA review size/S 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[chart power query] An error message occurs when user saves a chart using chart power query
7 participants