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

[logs] Dropping dt column #4587

Merged
merged 1 commit into from
Apr 11, 2018

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Mar 9, 2018

Whist looking through the Superset logs table I discovered that there was a misalignment of the date defined in the dt column vs dttm.

The reason for the inconsistency is dttm is defined in reference to the UTC timezone whereas dt use the date in reference to one's local timezone. One possible fix is

dt = Column(Date, default=ddatetime.utcnow().date())

however given that dt and dttm aren't using the same now instance, there's still no guarantee there will be consistency between these columns especially around 12:00:00 am UTC.

The fix here is simply to remove the dt column which is unnecessary as this information is completely defined by the dttm column. Note I also removed the deprecated activity_per_day which used the Log.dt column which is no longer used in the code.

Additionally tested by running:

superset db upgrade
superset db downgrade

to: @graceguo-supercat @michellethomas @mistercrunch @timifasubaa

@john-bodley john-bodley force-pushed the john-bodley-logs-remove-dt branch 3 times, most recently from b8a452a to acd146d Compare March 10, 2018 00:07
@mistercrunch
Copy link
Member

mistercrunch commented Mar 11, 2018

That's a breaking change though as dt may be used in charts/dashboards/etl . Personally I'm fine with breaking changes before we hit 1.0, but we need an UPDATING.md or equivalent with a list of breaking changes, motives and ways to handle them associated with versions. When releasing, we'd be pointing to the UPDATING.md notes on top of change log.

Here's what the Airflow UPDATING.md look like:
https://github.com/apache/incubator-airflow/blob/master/UPDATING.md

It can be simple at first, with just a list of version and related breaking changes and PR references.

@john-bodley john-bodley force-pushed the john-bodley-logs-remove-dt branch 2 times, most recently from 83e314b to 255b252 Compare March 12, 2018 23:47
@john-bodley
Copy link
Member Author

@mistercrunch I added an UPDATING.md file. Let me know if you want me to add more refinement beyond a simple list of breaking PRs.

@john-bodley john-bodley force-pushed the john-bodley-logs-remove-dt branch 2 times, most recently from 7226dff to 634791b Compare March 15, 2018 06:29
@codecov-io
Copy link

codecov-io commented Mar 15, 2018

Codecov Report

Merging #4587 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4587      +/-   ##
==========================================
+ Coverage   72.63%   72.64%   +0.01%     
==========================================
  Files         205      205              
  Lines       15403    15395       -8     
  Branches     1183     1183              
==========================================
- Hits        11188    11184       -4     
+ Misses       4212     4208       -4     
  Partials        3        3
Impacted Files Coverage Δ
superset/views/core.py 74.62% <ø> (+0.16%) ⬆️
superset/models/core.py 86.49% <100%> (-0.03%) ⬇️

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 41c158e...ad7556d. Read the comment docs.

@michellethomas
Copy link
Contributor

lgtm

@john-bodley john-bodley merged commit 7f1d754 into apache:master Apr 11, 2018
@john-bodley john-bodley deleted the john-bodley-logs-remove-dt branch April 11, 2018 01:34

def upgrade():
with op.batch_alter_table('logs') as batch_op:
batch_op.drop_column('dt')
Copy link
Member

Choose a reason for hiding this comment

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

@john-bodley let's avoid backward incompatible db migrations in the future as they require scheduling downtime to upgrade the app for people who use rolling deploys. This took our production down for a moment.

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 just to clarify what should have the logic been here? I'm simply asking to learn.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, maybe we could have just left the column there as NULL moving forward and add a note about it being deprecated.

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
timifasubaa pushed a commit to timifasubaa/incubator-superset that referenced this pull request May 31, 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.25.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.25.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants