Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

fix(time-format): remove week number #493

Merged
merged 2 commits into from
May 18, 2020

Conversation

ktmud
Copy link
Contributor

@ktmud ktmud commented May 15, 2020

🐛 Bug Fix

Most databases, including ISO standards, actually uses Monday as the first day of a week. The default week time grain in Superset (P1W) should translate to Monday-based weeks, too.

@ktmud ktmud requested a review from a team as a code owner May 15, 2020 17:20
@vercel
Copy link

vercel bot commented May 15, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/6yzv0sf7l
✅ Preview: https://superset-ui-git-fork-ktmud-time-format-week-base.superset.now.sh

@codecov
Copy link

codecov bot commented May 15, 2020

Codecov Report

Merging #493 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #493      +/-   ##
==========================================
- Coverage   22.72%   22.70%   -0.03%     
==========================================
  Files         276      276              
  Lines        6670     6668       -2     
  Branches      644      644              
==========================================
- Hits         1516     1514       -2     
  Misses       5115     5115              
  Partials       39       39              
Impacted Files Coverage Δ
...at/src/factories/getTimeFormatterForGranularity.ts 100.00% <ø> (ø)

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 5b42bc1...e21b843. Read the comment docs.

@michellethomas
Copy link
Contributor

Thanks I checked presto and it looks like the logic for presto is date_trunc('week', CAST({col} AS TIMESTAMP)) and when I tested it out it looks like week starts on Monday. Do we know if this is true for all supported db_engine_specs? It seems like if any of them evaluate to week start Sunday, this would be incorrect. Can we just show the date for the start of the week (to make sure we will always be accurate)?

@ktmud
Copy link
Contributor Author

ktmud commented May 15, 2020

Can we just show the date for the start of the week (to make sure we will always be accurate)?

That was fixed in #486 .

Do we know if this is true for all supported db_engine_specs?

I also checked Druid, MySQL, PostgreSQL, SQL Lite and some other databases, with the exception of MySQL and Drill, all others are using Monday-based weekdays.

Given how most businesses operate and that Monday-based is also the ISO standard, I propose we move forward with this change and change MySQL & Drill to be Monday-based, too.

cc @mistercrunch @john-bodley @kristw

@ktmud ktmud changed the title fix: week number is actually Monday based by default fix(time-format): change week number to be Monday based May 15, 2020
ktmud added 2 commits May 17, 2020 23:59
It is confusing and not really useful in most business settings.
@ktmud
Copy link
Contributor Author

ktmud commented May 18, 2020

Another option is to simply remove the week numbers, as: 1) it is not useful for most business settings anyway; 2) different businesses may have different definition of week-of-years.

@ktmud ktmud changed the title fix(time-format): change week number to be Monday based fix(time-format): remove week number May 18, 2020
@kristw
Copy link
Contributor

kristw commented May 18, 2020

I agree with removing Wxxx

@kristw kristw merged commit 0d13ff0 into apache-superset:master May 18, 2020
ktmud added a commit to ktmud/superset that referenced this pull request May 18, 2020
kristw pushed a commit to apache/superset that referenced this pull request May 18, 2020
* feat: bump superset-ui/time-format and big-number

This is for adding full dates to big number datetime formatted
by weekly interval.

* Bump time-format to v0.13.15

to bring in apache-superset/superset-ui#493
@michellethomas
Copy link
Contributor

Thanks for resolving this! This seems like a good solution.

auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* feat: bump superset-ui/time-format and big-number

This is for adding full dates to big number datetime formatted
by weekly interval.

* Bump time-format to v0.13.15

to bring in apache-superset/superset-ui#493
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants