Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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

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
# for free to join this conversation on GitHub. Already have an account? # 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