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: samples should not be timeseries #12796

Merged
merged 1 commit into from
Jan 28, 2021
Merged

Conversation

betodealmeida
Copy link
Member

SUMMARY

When we fetch samples for a given viz we need to ensure the is_timeseries attribute is false, otherwise Superset will add a group by on the temporal column, resuilting in an invalid column:

SELECT ts AS ts,
       name AS name,
       text AS text,
       DATE(ts) AS __timestamp  -- HERE
FROM
  (SELECT m.ts,
          c.name,
          m.text
   FROM messages m
   JOIN channels c ON m.channel_id = c.id) AS expr_qry
WHERE ts >= STR_TO_DATE('2020-10-27 00:00:00.000000', '%Y-%m-%d %H:%i:%s.%f')
  AND ts < STR_TO_DATE('2021-01-27 00:00:00.000000', '%Y-%m-%d %H:%i:%s.%f')
  AND name != 'github-notifications'
GROUP BY DATE(ts)  -- HERE
LIMIT 1000

The query above is invalid because the GROUP BY should contain all non-aggregation expressions from the SELECT.

Because of this, some charts fail to load their samples.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

Screenshot_2021-01-27 Messages per Channel(1)

After:

Screenshot_2021-01-27 Messages per Channel

TEST PLAN

Open chart and navigated to "DATA" -> "VIEW SAMPLES", now it works as expected.

ADDITIONAL INFORMATION

@codecov-io
Copy link

codecov-io commented Jan 27, 2021

Codecov Report

Merging #12796 (979f908) into master (044d1ae) will increase coverage by 4.40%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12796      +/-   ##
==========================================
+ Coverage   67.00%   71.40%   +4.40%     
==========================================
  Files        1022      534     -488     
  Lines       50047    19930   -30117     
  Branches     4914     4914              
==========================================
- Hits        33533    14232   -19301     
+ Misses      16390     5574   -10816     
  Partials      124      124              
Flag Coverage Δ
cypress 50.89% <ø> (ø)
javascript 61.69% <ø> (ø)
python ?

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

Impacted Files Coverage Δ
...igrations/versions/763d4b211ec9_fixing_audit_fk.py
superset/views/utils.py
superset/examples/world_bank.py
superset/datasets/filters.py
...s/versions/b6fa807eac07_make_names_non_nullable.py
superset/reports/api.py
superset/examples/countries.py
superset/models/dynamic_plugins.py
...t/annotation_layers/annotations/commands/delete.py
...perset/dashboards/commands/importers/dispatcher.py
... and 477 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 044d1ae...979f908. Read the comment docs.

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!

@villebro villebro merged commit 5a3c5b1 into apache:master Jan 28, 2021
@mistercrunch mistercrunch added 🍒 1.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.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/XS v1.0.1 🍒 1.0.1 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants