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

feat(explore): Allow using time formatter on temporal columns in data table #18569

Merged
merged 11 commits into from
Feb 9, 2022

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Feb 3, 2022

SUMMARY

Enable switching between displaying epoch (original value) and time formatted value (format: YYYY-mm-dd HH:MM:SS).

Temporal columns in data table (tabs "Data" and "Samples") now have a clickable "gear" icon, which opens a popover that allows switching between 2 display modes.

Selected display mode is specific to each column - user can select different display modes for each temporal column. The selection is also specific to datasource, meaning if there are temporal columns that have the same name in 2 datasources, those columns can have different display modes.

Selection is saved in local storage - when user reloads the page, changes hart config etc., it will be persisted.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-02-03.at.14.27.52.mov

TESTING INSTRUCTIONS

  1. Create a chart with some temporal columns
  2. Open data table below the chart - temporal columns should have "gear" icons on their left sides
  3. Click the icon and select "Formatted time"
  4. Verify that the displayed values for that column have changed from long numbers to human readable dates
  5. Reload the page, verify that your selections have been saved
  6. Switch between datasources, verify that the selections are specific for each datasource
  7. Perform the above testing instructions for charts using V1 API and legacy API

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

https://app.shortcut.com/preset/story/33355/explore-south-table-timestamp-column-in-data-panel-did-not-show-correctly

CC @rusackas @jinghua-qa @kasiazjc

@codecov
Copy link

codecov bot commented Feb 3, 2022

Codecov Report

Merging #18569 (e961149) into master (a80efa6) will decrease coverage by 0.08%.
The diff coverage is 67.48%.

❗ Current head e961149 differs from pull request most recent head 618a3ef. Consider uploading reports for the commit 618a3ef to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18569      +/-   ##
==========================================
- Coverage   66.29%   66.21%   -0.09%     
==========================================
  Files        1594     1596       +2     
  Lines       62623    62656      +33     
  Branches     6312     6314       +2     
==========================================
- Hits        41518    41488      -30     
- Misses      19456    19520      +64     
+ Partials     1649     1648       -1     
Flag Coverage Δ
javascript 51.31% <67.74%> (-0.03%) ⬇️
presto ?

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% <ø> (ø)
...rset-frontend/src/components/ListView/ListView.tsx 96.25% <ø> (ø)
...ols/DndColumnSelectControl/ColumnSelectPopover.tsx 3.94% <0.00%> (+0.05%) ⬆️
...ColumnSelectControl/ColumnSelectPopoverTrigger.tsx 65.51% <ø> (+1.00%) ⬆️
.../controls/DndColumnSelectControl/OptionWrapper.tsx 63.41% <ø> (ø)
...et-frontend/src/explore/reducers/exploreReducer.js 25.71% <0.00%> (-7.62%) ⬇️
...t-frontend/src/explore/reducers/getInitialState.ts 0.00% <ø> (ø)
superset-frontend/src/utils/localStorageHelpers.ts 90.00% <ø> (ø)
superset/charts/api.py 85.93% <ø> (ø)
superset/charts/schemas.py 99.34% <ø> (ø)
... and 26 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 a80efa6...618a3ef. Read the comment docs.

@kgabryje
Copy link
Member Author

kgabryje commented Feb 3, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2022

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

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

First pass comments. Works really nicely! A minor wording change proposal + I recommend also adding it to legacy charts due to the minor needed changes.

superset/views/core.py Outdated Show resolved Hide resolved
superset/viz.py Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Feb 4, 2022
@kgabryje
Copy link
Member Author

kgabryje commented Feb 4, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2022

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

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Two last comments, other than that LGTM!

superset/views/core.py Outdated Show resolved Hide resolved
@zhaoyongjie
Copy link
Member

zhaoyongjie commented Feb 7, 2022

Thanks for your hard work. Before I reviewed the code, I did some experiments.

  1. Let us use this SQL(PG) to make a virtual dataset, then open it in explore.
select
  '2022-01-03 00:00:00'::TIMESTAMP as datetime0,
  '2022-02-07 00:00:00'::TIMESTAMP as datetime1
UNION ALL
  SELECT '2022-01-04 00:00:00'::TIMESTAMP,
  '2022-02-08 00:00:00'::TIMESTAMP
UNION ALL
  SELECT '2022-01-05 00:00:00'::TIMESTAMP,
  '2022-02-09 00:00:00'::TIMESTAMP
UNION ALL
  SELECT '2022-01-06 00:00:00'::TIMESTAMP,
  '2022-02-10 00:00:00'::TIMESTAMP
UNION ALL
  SELECT '2022-01-07 00:00:00'::TIMESTAMP,
  '2022-02-11 00:00:00'::TIMESTAMP
UNION ALL
  SELECT '2022-01-08 00:00:00'::TIMESTAMP,
  '2022-02-12 00:00:00'::TIMESTAMP
UNION ALL
  SELECT '2022-01-09 00:00:00'::TIMESTAMP,
  '2022-02-13 00:00:00'::TIMESTAMP
UNION ALL
  SELECT '2022-01-10 00:00:00'::TIMESTAMP,
  '2022-02-14 00:00:00'::TIMESTAMP
UNION ALL
  SELECT '2022-01-11 00:00:00'::TIMESTAMP,
  '2022-02-15 00:00:00'::TIMESTAMP
UNION ALL
  SELECT '2022-01-12 00:00:00'::TIMESTAMP,
  '2022-02-16 00:00:00'::TIMESTAMP
UNION ALL
  SELECT '2022-01-13 00:00:00'::TIMESTAMP,
  '2022-02-17 00:00:00'::TIMESTAMP
UNION ALL
  SELECT '2022-01-14 00:00:00'::TIMESTAMP,
  '2022-02-18 00:00:00'::TIMESTAMP
UNION ALL
  SELECT '2022-01-15 00:00:00'::TIMESTAMP,
  '2022-02-19 00:00:00'::TIMESTAMP
UNION ALL
  SELECT '2022-01-16 00:00:00'::TIMESTAMP,
  '2022-02-20 00:00:00'::TIMESTAMP
  1. Select table, put datatime0 and datetime1 to groupby control, put count(*) to metric, select month grain

  2. viz result and query result is different.

image

This problem was not actually introduced by this PR, but it is a long-standing one.

let us check SQL, we can see time function only apply on main time column, while datetime1 wrongly uses the time drill-down function in the frontend.

IMO, We'd better fix the original issue before we merge this PR into master.

SELECT DATE_TRUNC('month', datetime0) AS datetime0,
       datetime1 AS datetime1,
       count(*) AS count
FROM
  (select '2022-01-03 00:00:00'::TIMESTAMP as datetime0,
          '2022-02-07 00:00:00'::TIMESTAMP as datetime1
   UNION ALL SELECT '2022-01-04 00:00:00'::TIMESTAMP,
                    '2022-02-08 00:00:00'::TIMESTAMP
   UNION ALL SELECT '2022-01-05 00:00:00'::TIMESTAMP,
                    '2022-02-09 00:00:00'::TIMESTAMP
   UNION ALL SELECT '2022-01-06 00:00:00'::TIMESTAMP,
                    '2022-02-10 00:00:00'::TIMESTAMP
   UNION ALL SELECT '2022-01-07 00:00:00'::TIMESTAMP,
                    '2022-02-11 00:00:00'::TIMESTAMP
   UNION ALL SELECT '2022-01-08 00:00:00'::TIMESTAMP,
                    '2022-02-12 00:00:00'::TIMESTAMP
   UNION ALL SELECT '2022-01-09 00:00:00'::TIMESTAMP,
                    '2022-02-13 00:00:00'::TIMESTAMP
   UNION ALL SELECT '2022-01-10 00:00:00'::TIMESTAMP,
                    '2022-02-14 00:00:00'::TIMESTAMP
   UNION ALL SELECT '2022-01-11 00:00:00'::TIMESTAMP,
                    '2022-02-15 00:00:00'::TIMESTAMP
   UNION ALL SELECT '2022-01-12 00:00:00'::TIMESTAMP,
                    '2022-02-16 00:00:00'::TIMESTAMP
   UNION ALL SELECT '2022-01-13 00:00:00'::TIMESTAMP,
                    '2022-02-17 00:00:00'::TIMESTAMP
   UNION ALL SELECT '2022-01-14 00:00:00'::TIMESTAMP,
                    '2022-02-18 00:00:00'::TIMESTAMP
   UNION ALL SELECT '2022-01-15 00:00:00'::TIMESTAMP,
                    '2022-02-19 00:00:00'::TIMESTAMP
   UNION ALL SELECT '2022-01-16 00:00:00'::TIMESTAMP,
                    '2022-02-20 00:00:00'::TIMESTAMP) AS virtual_table
GROUP BY DATE_TRUNC('month', datetime0),
         datetime1
ORDER BY count DESC
LIMIT 1000;

@villebro
Copy link
Member

villebro commented Feb 7, 2022

@zhaoyongjie the reason for this is that existing users are currently relying on the timegrain only applying to the primary temporal column. See the discussion here: #17239 We're actually currently exploring the possibility of introducing a time grain option in the column control, after which we will no longer be restricted to only having a single time grained temporal column.

@zhaoyongjie
Copy link
Member

zhaoyongjie commented Feb 7, 2022

@zhaoyongjie the reason for this is that existing users are currently relying on the timegrain only applying to the primary temporal column. See the discussion here: #17239 We're actually currently exploring the possibility of introducing a time grain option in the column control, after which we will no longer be restricted to only having a single time grained temporal column.

@villebro Thanks for the mention! I applied time format in the customize panel to resolve the inconsistency.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM!

@kgabryje kgabryje merged commit 830f2e7 into apache:master Feb 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 9, 2022

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 🚢 1.5.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/XL 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants