feat(time-format): improve support for formatting with granularity in mind #509

kristw · 2020-05-19T18:20:13Z

🏆 Enhancements

What are the changes?

Extends core time formatting functions to include granularity support

getTimeFormatter(formatId) to getTimeFormatter(formatId?: string, granularity?: TimeGranularity)
formatTime(formatId, value) to formatTime(formatId: string | undefined, value: Date | null | undefined, granularity?: TimeGranularity)

New APIs

getTimeRangeFormatter(formatId?: string)
formatTimeRange(formatId: string | undefined, range: (Date | null | undefined)[])

Others

Convert the granularity string into constants.

Explanation of technical changes

When a time includes granularity, it means the given time is not a single point in time, but a time range. The changes in this PR includes logic for reconstructing the time range from time and granularity. The current logic assumes that the input time will be a valid output from database based on the granularity. For example, if the granularity is P1M (monthly), the database will always return the first day of the month.

Formatting a time point with granularity then follows these steps:

Convert time and granularity into a time range.
Pass the output time range to a TimeRangeFormatter, along with formatId (~d3 format string) or if no format is specified, look up default format string from a lookup table based on granularity (TimeFormatsForGranularity)
The TimeRangeFormatter at the moment is quite naive. Just try to apply same formatting to the start and end of the time range. If both start and end have the same output then return just start, otherwise return ${start} - ${end}.

See more concrete examples in the unit tests.

Future plans

TimeRangeFormatter can also have its own registry in case developer wants to override how certain format works for range.
Same with the TimeFormatsForGranularity, which each application may have different opinion about default settings.

vercel · 2020-05-19T18:20:16Z

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/q2ddcb6he
✅ Preview: https://superset-ui-git-kristw-time-format.superset.now.sh

codecov · 2020-05-19T18:21:12Z

Codecov Report

Merging #509 into master will increase coverage by 0.68%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #509      +/-   ##
==========================================
+ Coverage   22.63%   23.32%   +0.68%     
==========================================
  Files         281      286       +5     
  Lines        6692     6752      +60     
  Branches      648      672      +24     
==========================================
+ Hits         1515     1575      +60     
  Misses       5138     5138              
  Partials       39       39

Impacted Files	Coverage Δ
...-time-format/src/factories/createMultiFormatter.ts	`100.00% <ø> (ø)`
...ckages/superset-ui-time-format/src/utils/d3Time.ts	`100.00% <ø> (ø)`
...et-ui-time-format/src/TimeFormatsForGranularity.ts	`100.00% <100.00%> (ø)`
...kages/superset-ui-time-format/src/TimeFormatter.ts	`100.00% <100.00%> (ø)`
...-time-format/src/TimeFormatterRegistrySingleton.ts	`100.00% <100.00%> (ø)`
.../superset-ui-time-format/src/TimeRangeFormatter.ts	`100.00% <100.00%> (ø)`
packages/superset-ui-time-format/src/types.ts	`100.00% <100.00%> (ø)`
...es/superset-ui-time-format/src/utils/createTime.ts	`100.00% <100.00%> (ø)`
...format/src/utils/createTimeRangeFromGranularity.ts	`100.00% <100.00%> (ø)`
...set-ui-time-format/src/utils/stringifyTimeInput.ts	`100.00% <100.00%> (ø)`
... and 6 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 4ceacfb...2150aa8. Read the comment docs.

kristw · 2020-05-19T18:22:46Z

packages/superset-ui-time-format/src/factories/createMultiFormatter.ts

@@ -1,5 +1,5 @@
 import { utcFormat, timeFormat } from 'd3-time-format';
-import { utcUtils, localTimeUtils } from '../utils';
+import { utcUtils, localTimeUtils } from '../utils/d3Time';


move and rename file

kristw · 2020-05-19T18:23:44Z

packages/superset-ui-time-format/src/types.ts

+/**
+ * search for `builtin_time_grains` in incubator-superset/superset/db_engine_specs/base.py
+ */
+export const TimeGranularity = {


Make this also a constant to avoid having to type the awkward granularity string for WEEK_ENDING_xxx

ktmud

In theory, this is definitely a nice feature to have, but I'm worried whether it will lead to potential confusions if we do date truncating on both backend and frontend----users might see inconsistent datetime in charts and raw data.

What is the use case for the new API?

packages/superset-ui-time-format/src/types.ts

packages/superset-ui-time-format/src/utils/createDate.ts

packages/superset-ui-time-format/src/utils/createTimeRangeFromGranularity.ts

ktmud · 2020-05-19T18:38:10Z

packages/superset-ui-time-format/src/utils/createTimeRangeFromGranularity.ts

+  }
+
+  // Year
+  return deductOneMs(createDate({ year: year + 1, useLocalTime }));


How you thought of utilize d3-time intervals? https://github.com/d3/d3-time#interval_round

That could work. Will look into it.

Tried it. The floor and ceil functions work if the value is in the middle of the range but not so useful for the case we are handlings that the inputs are already at the beginning of the range. Will have to add offset before calling ceil. It also ceils to YYYY-mm-dd 00:00:00.000 instead of YYYY-mm-dd 23.59.59.999 that I would like.

kristw · 2020-05-19T18:48:02Z

In theory, this is definitely a nice feature to have, but I'm worried whether it will lead to potential confusions if we do date truncating on both backend and frontend----users might see inconsistent datetime in charts and raw data.

Could you elaborate on the truncation? I am not sure I understand what you mean.

ktmud · 2020-05-19T19:05:53Z

In theory, this is definitely a nice feature to have, but I'm worried whether it will lead to potential confusions if we do date truncating on both backend and frontend----users might see inconsistent datetime in charts and raw data.

Could you elaborate on the truncation? I am not sure I understand what you mean.

computeEndTimeFromGranularity seems to be truncating the startTime based on granularity. I was worried since the backend already truncates timestamp based on time granularities, would there be a case when some piece of code passes the truncated timestamps to the frontend API with a different granularity?

kristw · 2020-05-20T17:21:49Z

computeEndTimeFromGranularity seems to be truncating the startTime based on granularity. I was worried since the backend already truncates timestamp based on time granularities, would there be a case when some piece of code passes the truncated timestamps to the frontend API with a different granularity?

computeEndTimeFromGranularity computes the endTime. I don't think it touches startTime at all. This is computing true time range for each data point.

For example, when a data point is returned from database, it seems like a single point in time.

{ time: 2010-04-01, value: 300 }

However, it is rarely a single point in time. If this query has MONTH granularity, the data point above actually means the entire April.

This is what the actual data point is.

{ time: [2010-04-01, 2010-04-30], value: 300 }

so computeEndTimeFromGranularity takes 2010-04-01 and MONTH and returns the end time 2010-04-30 just for formatting.

Now we know the time range is [2010-04-01, 2010-04-30], it depends on the format string.

If the format is %Y-%m this will be 2010-04 - 2010-04 which looks redundant, so we can merge and return 2010-04
If the format string is %Y-%m-%d this will be 2010-04-01 - 2010-04-30

ktmud · 2020-05-20T17:46:02Z

computeEndTimeFromGranularity computes the endTime. I don't think it touches startTime at all. This is computing true time range for each data point.

I mean It calculates an endTime based on truncated startTime, since it only takes parts of startTime. I can see why this function can be useful, but would there be a case the truncating is done differently on the frontend and backend? For example, some db engine may round dates up instead of round-down? Or when backend rounded the dates to weeks, but the frontend still uses days because some chart forgot to pass the granularity to the formatter?

Maybe I was just overthinking.

kristw · 2020-05-20T17:58:46Z

For example, some db engine may round dates up instead of round-down?

The granularity WEEK_ENDING_xxx rounds date up and this function will react accordingly. I am not aware of other cases that it does not round down. (This would be an even broader issue that already exists if some database do round up by default.) The current code handles all granularity listed in the superset db_engine.

Or when backend rounded the dates to weeks, but the frontend still uses days because some chart forgot to pass the granularity to the formatter?

This is already the current behavior I think. This feature allows it to be more correct if the granularity is passed to the formatter. It is an opt-in feature and should not affect charts (except BigNumber) if we don't make any change to them.

kristw requested a review from a team as a code owner May 19, 2020 18:20

kristw commented May 19, 2020

View reviewed changes

ktmud reviewed May 19, 2020

View reviewed changes

kristw added 9 commits May 20, 2020 12:45

feat(time-format): add support for granularity

d56fb7d

feat: create time range from granularity

a23b906

fix: update format

7ef1c36

wip

19b6a54

feat: refactor getFormatter

9785264

feat: reconcile api

3eda4d8

test: add unit tests

7418488

refactor: clean up

4065e45

refactor: createTime

08ac1b5

kristw force-pushed the kristw--time-format branch from 2c064d1 to 08ac1b5 Compare May 20, 2020 20:06

vercel bot deployed to Preview May 20, 2020 20:06 View deployment

refactor: improve end time computation to be daylight saving compatible

2150aa8

vercel bot deployed to Preview May 20, 2020 21:37 View deployment

ktmud approved these changes May 22, 2020

View reviewed changes

kristw merged commit 2d8afa8 into master May 22, 2020

delete-merged-branch bot deleted the kristw--time-format branch May 22, 2020 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(time-format): improve support for formatting with granularity in mind #509

feat(time-format): improve support for formatting with granularity in mind #509

kristw commented May 19, 2020 •

edited

Loading

vercel bot commented May 19, 2020 •

edited

Loading

codecov bot commented May 19, 2020 •

edited

Loading

kristw May 19, 2020

kristw May 19, 2020

ktmud left a comment

ktmud May 19, 2020

kristw May 19, 2020

kristw May 20, 2020 •

edited

Loading

kristw commented May 19, 2020

ktmud commented May 19, 2020

kristw commented May 20, 2020

ktmud commented May 20, 2020

kristw commented May 20, 2020 •

edited

Loading

feat(time-format): improve support for formatting with granularity in mind #509

feat(time-format): improve support for formatting with granularity in mind #509

Conversation

kristw commented May 19, 2020 • edited Loading

What are the changes?

Extends core time formatting functions to include granularity support

New APIs

Others

Explanation of technical changes

Future plans

vercel bot commented May 19, 2020 • edited Loading

codecov bot commented May 19, 2020 • edited Loading

Codecov Report

kristw May 19, 2020

Choose a reason for hiding this comment

kristw May 19, 2020

Choose a reason for hiding this comment

ktmud left a comment

Choose a reason for hiding this comment

ktmud May 19, 2020

Choose a reason for hiding this comment

kristw May 19, 2020

Choose a reason for hiding this comment

kristw May 20, 2020 • edited Loading

Choose a reason for hiding this comment

kristw commented May 19, 2020

ktmud commented May 19, 2020

kristw commented May 20, 2020

ktmud commented May 20, 2020

kristw commented May 20, 2020 • edited Loading

kristw commented May 19, 2020 •

edited

Loading

vercel bot commented May 19, 2020 •

edited

Loading

codecov bot commented May 19, 2020 •

edited

Loading

kristw May 20, 2020 •

edited

Loading

kristw commented May 20, 2020 •

edited

Loading