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

[epoch] Remove non-UTC epoch logic #7667

Merged

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Jun 6, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

As @agrawaldevesh correctly identified in #6721 previously we were computing the Unix timestamp for the right-hand-side (RHS) of the temporal filter condition using the local time zone as opposed to UTC which is the definition of a Unix timestamp (or epoch).

@agrawaldevesh's change was behind a feature flag and disabled by default however this clearly is a bug and I sense we should remedy the problem by merely replacing the previously incorrect logic. Note I strongly believe users were probably unaware of the issue as Unix timestamps aren't human readable.

TEST PLAN

CI.

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

to: @agrawaldevesh @betodealmeida @michellethomas @mistercrunch @villebro

@john-bodley
Copy link
Member Author

#7656

@codecov-io
Copy link

codecov-io commented Jun 6, 2019

Codecov Report

Merging #7667 into master will decrease coverage by 8.34%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7667      +/-   ##
==========================================
- Coverage   73.92%   65.58%   -8.35%     
==========================================
  Files         106      435     +329     
  Lines       11444    21749   +10305     
  Branches        0     2394    +2394     
==========================================
+ Hits         8460    14264    +5804     
- Misses       2984     7364    +4380     
- Partials        0      121     +121
Impacted Files Coverage Δ
superset/config.py 93.97% <ø> (-0.08%) ⬇️
superset/connectors/sqla/models.py 82.39% <75%> (+0.41%) ⬆️
superset/connectors/sqla/views.py 63.15% <0%> (-2.14%) ⬇️
superset/connectors/druid/views.py 66.21% <0%> (-1.53%) ⬇️
superset/views/core.py 72.87% <0%> (-0.19%) ⬇️
superset/db_engine_specs/kylin.py
superset/db_engine_specs/mysql.py
superset/db_engine_specs/mssql.py
superset/db_engine_specs/sqlite.py
... and 373 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 541db94...e666244. 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, but perhaps add a comment in the changelog in case someone has logic depending on the incorrect epoch format?

@john-bodley john-bodley force-pushed the john-bodley--deprecate-non-utc-epoch branch from e93f33b to f80712b Compare June 6, 2019 22:45
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.

LGTM

@@ -23,6 +23,9 @@ assists people when migrating to a new version.

## Next Version

* [7667](https://github.com/apache/incubator-superset/pull/7667): a change to
Copy link
Member

Choose a reason for hiding this comment

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

nit: remove the "to" before reference

a change to make Unix (epoch) timestamps reference UTC as opposed to the local time zone.

@john-bodley john-bodley force-pushed the john-bodley--deprecate-non-utc-epoch branch from 2f06959 to d14586c Compare June 11, 2019 00:01
@john-bodley
Copy link
Member Author

@agrawaldevesh are you onboard with this change?

@agrawaldevesh
Copy link
Contributor

agrawaldevesh commented Jun 11, 2019 via email

@john-bodley john-bodley force-pushed the john-bodley--deprecate-non-utc-epoch branch from d14586c to 6dfdde0 Compare June 11, 2019 20:24
@john-bodley john-bodley force-pushed the john-bodley--deprecate-non-utc-epoch branch 2 times, most recently from 3d290e4 to e666244 Compare June 11, 2019 21:10
@john-bodley john-bodley merged commit 8d6257a into apache:master Jun 12, 2019
@john-bodley john-bodley deleted the john-bodley--deprecate-non-utc-epoch branch June 12, 2019 05:34
shmsr added a commit to ThalesGroup/incubator-superset that referenced this pull request Nov 11, 2019
PR apache#8450: Making client time use UTC as the local time
PR apache#7667: Remove non-UTC epoch logic
shmsr added a commit to ThalesGroup/incubator-superset that referenced this pull request Nov 11, 2019
PR apache#8450: Making client time use UTC as the local time
PR apache#7667: Remove non-UTC epoch logic
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 28, 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/S 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants