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] Enforcing main dttm column #5584

Merged
merged 1 commit into from
Aug 13, 2018

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Aug 8, 2018

This PR ensures that if defined the "Main Datetime Column" is used as the default value. Previously this value was being ignored and thus for datasource with multiple temporal columns a sub-optimal choice may occur, i.e., using a non-partitioned column.

to: @GabeLoins @graceguo-supercat @michellethomas @mistercrunch @williaster

closes #3629

@john-bodley john-bodley force-pushed the john-bodley-main-dttm-column branch 3 times, most recently from f07b2ba to a322ba2 Compare August 8, 2018 17:17
@codecov-io
Copy link

codecov-io commented Aug 8, 2018

Codecov Report

Merging #5584 into master will decrease coverage by <.01%.
The diff coverage is 30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5584      +/-   ##
==========================================
- Coverage   63.62%   63.62%   -0.01%     
==========================================
  Files         359      359              
  Lines       22820    22823       +3     
  Branches     2533     2534       +1     
==========================================
  Hits        14520    14520              
- Misses       8285     8288       +3     
  Partials       15       15
Impacted Files Coverage Δ
superset/connectors/sqla/models.py 78.96% <100%> (+0.04%) ⬆️
superset/assets/src/explore/controls.jsx 45.92% <22.22%> (-1.45%) ⬇️

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 50981db...4160819. Read the comment docs.

@rumbin
Copy link
Contributor

rumbin commented Aug 11, 2018

This PR would remedy #3629, I suppose.

Thanks a lot!

@@ -846,6 +841,12 @@ export const controls = {
const newState = {};
if (state.datasource) {
newState.options = state.datasource.columns.filter(c => c.is_dttm);
newState.default = null;
Copy link
Member

Choose a reason for hiding this comment

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

Not related to your PR: props would be a better variable name than newState in this method.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mistercrunch I've renamed newState to props.

@mistercrunch
Copy link
Member

LGTM

@john-bodley john-bodley merged commit 2685ab4 into apache:master Aug 13, 2018
@john-bodley john-bodley deleted the john-bodley-main-dttm-column branch August 13, 2018 20:55
john-bodley added a commit to john-bodley/superset that referenced this pull request Aug 13, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 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 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Explore] Regression: Main Datetime Column not chosen automatically
5 participants