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: change default time range from sql lab to no filter #9439

Closed
wants to merge 1 commit into from

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented Apr 1, 2020

CATEGORY

  • Enhancement (new features, refinement)

SUMMARY

Currently when clicking on "Explore" button in SQL Lab results, it defaults the time range to "100 years ago : infinity", which is basically equivalent to no filter for most datasources. However, it has implication on how the x-axis will be plotted in certain charts (e.g., recently, the Big Number chart introduced a fixed range option).

This PR changes the default to "No filter" so the time range filter can be more properly utilized.

There is also a global default value "Last week", which is applied when users create a new chart from /chart/add. We are not changing this default value in this PR.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

When clicking on Explore in SQL Lab:

image

Before

image

After

image

TEST PLAN

  1. Go to any query results in SQL Lab
  2. Click on "Explore"

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

REVIEWERS

@kristw

@codecov-io
Copy link

Codecov Report

Merging #9439 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9439   +/-   ##
=======================================
  Coverage   59.03%   59.03%           
=======================================
  Files         379      379           
  Lines       12181    12181           
  Branches     3010     3010           
=======================================
  Hits         7191     7191           
  Misses       4809     4809           
  Partials      181      181           
Impacted Files Coverage Δ
...end/src/SqlLab/components/ExploreResultsButton.jsx 73.75% <ø> (ø)

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 893c955...7a36f71. Read the comment docs.

@ktmud ktmud force-pushed the sql-lab-explore-default-no-filter branch 3 times, most recently from ce14124 to 5df6d8c Compare April 3, 2020 19:39
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

one comment around code quality. it's not really blocking, but it seems worth the couple minutes it would take now to address

@@ -139,8 +139,8 @@ class ExploreResultsButton extends React.PureComponent {
datasource: `${data.table_id}__table`,
metrics: [],
groupby: [],
time_range: 'No filter',
Copy link
Member

Choose a reason for hiding this comment

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

a bit weird that this is just plain text as opposed to null or something. I don't think fixing that is required right now, but i think moving "No filter" to a constant and using that constant it here would be reasonable. It looks like "No filter" is used in a few other places too (tests, other components) so maybe you could replace those your new constant too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree this is not a good practice. There seems to be many other hard-coded time periods, as well. I am not comfortable with refactoring while changing the default or refactoring just the 'No filter'. Maybe that refactoring can happen in a later PR?

Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

sure, let's make sure we pull this out as a constant in a future PR then

@ktmud
Copy link
Member Author

ktmud commented Apr 6, 2020

https://travis-ci.org/github/apache/incubator-superset/builds/670726287?utm_source=github_status&utm_medium=notification

CI has passed but GitHub still shows pending? Can we merge regardless?

@kristw kristw closed this Apr 7, 2020
@kristw kristw reopened this Apr 7, 2020
Change from "100 years go" to "no filter".

100 years ago is basically equivalent to no filter, but has
implications on how x-axis is plotted on certain charts (e.g. Big
Number).
@ktmud ktmud force-pushed the sql-lab-explore-default-no-filter branch from 5df6d8c to 127aaac Compare April 7, 2020 00:04
@ktmud ktmud closed this Apr 7, 2020
@ktmud ktmud reopened this Apr 7, 2020
@ktmud ktmud mentioned this pull request Apr 7, 2020
@ktmud ktmud closed this Apr 7, 2020
@ktmud ktmud deleted the sql-lab-explore-default-no-filter branch April 7, 2020 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants