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-sqllab): make that Timestamp column keep the Is temporal … #19010

Merged
merged 1 commit into from
Mar 25, 2022

Conversation

prosdev0107
Copy link
Contributor

@prosdev0107 prosdev0107 commented Mar 3, 2022

SUMMARY

Overwriting an existing virtual dataset removes the Is temporal flag from columns

How to reproduce the bug

  1. Navigate to the SQL Lab.
  2. Run a SELECT statement including a timestamp column.
  3. Click on EXPLORE.
  4. Create a new dataset.
  5. Note that the timestamp column is available to be selected on the time column drop-down.
  6. Navigate back to the SQL Lab.
  7. Run the same query again.
  8. Click on EXPLORE.
  9. Choose to overwrite the dataset you have just created.
  10. Note that the timestamp column is not available to be used on the time column drop-down.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:
image

AFTER:
image

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 Mar 3, 2022

Codecov Report

Merging #19010 (de3dc69) into master (be88cb9) will increase coverage by 0.20%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #19010      +/-   ##
==========================================
+ Coverage   66.58%   66.78%   +0.20%     
==========================================
  Files        1641     1643       +2     
  Lines       63548    65287    +1739     
  Branches     6424     7064     +640     
==========================================
+ Hits        42312    43603    +1291     
- Misses      19555    19936     +381     
- Partials     1681     1748      +67     
Flag Coverage Δ
javascript 52.42% <0.00%> (+1.04%) ⬆️

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

Impacted Files Coverage Δ
...frontend/src/SqlLab/components/ResultSet/index.tsx 50.73% <0.00%> (ø)
...ts/nativeFilters/FilterBar/FilterControls/utils.ts 57.14% <0.00%> (-12.86%) ⬇️
...Filters/FilterBar/FilterControls/FilterControl.tsx 77.35% <0.00%> (-4.79%) ⬇️
...oard/components/nativeFilters/FilterCard/Styles.ts 80.00% <0.00%> (-4.62%) ⬇️
...d/components/nativeFilters/FilterCard/ScopeRow.tsx 69.56% <0.00%> (-3.17%) ⬇️
...perset-frontend/src/addSlice/AddSliceContainer.tsx 58.62% <0.00%> (-3.15%) ⬇️
superset-frontend/src/explore/controls.jsx 27.94% <0.00%> (-3.10%) ⬇️
...et-frontend/src/explore/controlPanels/sections.tsx 83.33% <0.00%> (-2.39%) ⬇️
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx 57.62% <0.00%> (-1.39%) ⬇️
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx 76.56% <0.00%> (-1.02%) ⬇️
... and 56 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 be88cb9...de3dc69. Read the comment docs.

@rusackas rusackas requested a review from villebro March 3, 2022 19:35
@rusackas
Copy link
Member

rusackas commented Mar 3, 2022

It might be use to add repro steps to the PR description for historical purposes, and for anyone who wants to review/test. In this case:

  1. Navigate to the SQL Lab.
  2. Run a SELECT statement including a timestamp column.
  3. Click on EXPLORE.
  4. Create a new dataset.
  5. Note that the timestamp column is available to be selected on the time column drop-down.
  6. Navigate back to the SQL Lab.
  7. Run the same query again.
  8. Click on EXPLORE.
  9. Choose to overwrite the dataset you have just created.
  10. Note that the timestamp column is not available to be used on the time column drop-down.

Also, is it possible to add a test for this?

@prosdev0107
Copy link
Contributor Author

It might be use to add repro steps to the PR description for historical purposes, and for anyone who wants to review/test. In this case:

  1. Navigate to the SQL Lab.
  2. Run a SELECT statement including a timestamp column.
  3. Click on EXPLORE.
  4. Create a new dataset.
  5. Note that the timestamp column is available to be selected on the time column drop-down.
  6. Navigate back to the SQL Lab.
  7. Run the same query again.
  8. Click on EXPLORE.
  9. Choose to overwrite the dataset you have just created.
  10. Note that the timestamp column is not available to be used on the time column drop-down.

Also, is it possible to add a test for this?

@prosdev0107 prosdev0107 closed this Mar 4, 2022
@prosdev0107 prosdev0107 reopened this Mar 4, 2022
@prosdev0107
Copy link
Contributor Author

@evans
I just updated the columns of request params in the update the dataset api of superset.
And now the unit test isn't developed for this and so I am wondering if I can add the unit test for this.

@rusackas
Copy link
Member

rusackas commented Mar 7, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2022

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

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

In this PR, there seems to be a regression, with the following steps:

  1. I pick a dataset, like cleaned_sales_data
  2. I can open that in explore, and verify that order_date is available in the Time section of the controls as the Time Column
  3. I go to SQL Editor, and do SELECT * from cleaned_sales_data
  4. I click the Explore button, and save the dataset along the way
  5. In the Time section, the order_date field is no longer available in the Time Column input

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

I meant for that last comment to be a change request, not an approval 🤦

@prosdev0107
Copy link
Contributor Author

prosdev0107 commented Mar 10, 2022

@rusackas .
I followed the steps in last comment but I am not able to reproduce it.
And so I like to share the my screen recording file.
https://drive.google.com/file/d/1Y8p7bqvrWnk7KlqjPC4I5lfrHemSEwOO/view?usp=sharing

@rusackas
Copy link
Member

Interesting. I'm able to reproduce on the ephemeral environment, but not when I check out the PR to my local env. I seem to recall some changes being made to the ephemeral env configuration since this was opened, so I'll trust the local testing a little more and approve this :D

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM

@rusackas rusackas merged commit 4463586 into apache:master Mar 25, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

results.selected_columns.map(d => ({ column_name: d.name })),
results.selected_columns.map(d => ({
column_name: d.name,
is_dttm: d.is_date,
Copy link
Member

Choose a reason for hiding this comment

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

seems like is_dttm and is_date is inconsistency... Let me look at more.

villebro pushed a commit that referenced this pull request Apr 3, 2022
…flagged when overwriting (#19010)

(cherry picked from commit 4463586)
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
@mistercrunch mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 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 lts-v1 size/XS 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants