feat(core): add applied_time_extras to QueryObject by villebro · Pull Request #809 · apache-superset/superset-ui

villebro · 2020-10-16T13:46:08Z

🏆 Enhancements
Add applied_time_extras to QueryObject to be able to identify which filters have been set/overridden by extra_filters. Please see apache/superset#11325 for additional context.

vercel · 2020-10-16T13:46: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/57hllvjrw
✅ Preview: https://superset-ui-git-villebro-applied-time-extras.superset.vercel.app

codecov · 2020-10-16T13:53:44Z

Codecov Report

Merging #809 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #809   +/-   ##
=======================================
  Coverage   25.57%   25.57%           
=======================================
  Files         359      359           
  Lines        7949     7949           
  Branches     1056     1055    -1     
=======================================
  Hits         2033     2033           
  Misses       5796     5796           
  Partials      120      120

Impacted Files	Coverage Δ
...ckages/superset-ui-core/src/query/extractExtras.ts	`100.00% <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 298b59f...4127393. Read the comment docs.

pkdotson

LGTM!

villebro · 2020-10-16T16:50:27Z

Thanks @pkdotson !

ktmud

Added a couple of naming suggestions. I'm also not fully getting what is this used for. Can you also add more context on how is this going to be used in superset-frontend code (maybe a linked PR?) and what are we constrained with if we don't do this?

ktmud · 2020-10-16T17:02:53Z

packages/superset-ui-core/src/query/extractExtras.ts

      const queryField = reservedColumnsToQueryField[filter.col];
      partialQueryObject[queryField] = filter.val;
+      // @ts-ignore
+      partialQueryObject.applied_time_extras[filter.col] = filter.val;


Instead of ts-ignore, can use type assertion somewhere to make the typings compatible?

partialQueryObject.applied_time_extras[filter.col as QueryObjectAppliedTimeExtraKey] = filter.val;

ktmud · 2020-10-16T17:04:45Z

packages/superset-ui-core/src/query/types/Query.ts

  where?: string;
 }>;

+export type QueryObjectAppliedTimeExtraKey =


Can we rename this to something more generic such as TimeColumnConfigKey? Also, maybe put this into Time.ts ?

ktmud · 2020-10-16T17:07:09Z

packages/superset-ui-core/src/query/extractExtras.ts

+    applied_time_extras: {},
  };

  const reservedColumnsToQueryField: Record<string, keyof QueryObject> = {


Looks like this could be Record<QueryObjectAppliedTimeExtraKey, keyof QueryObject> or Record<TimeColumnConfigKey, keyof QueryObject>.

ktmud · 2020-10-16T17:16:21Z

packages/superset-ui-core/src/query/types/Query.ts

 };

 export type QueryObject = {
+  /** Extra filters that have been applied to the query object */


This comment doesn't seem to indicate how is this is different from extra.

villebro · 2020-10-16T17:52:52Z

Thanks for comments @ktmud , I'll add some more context to make this clearer and do the suggested code improvements. This is something that is needed to be able to complete the p0 filter indicator feature (filter indicators will otherwise not work with the chart data endpoint). I envision this and other related structures, namely how ad-hoc filters are transformed into simple filters + static wheres and havings will need to change soon when we start working on native filter components. Therefore I was hoping to make this slightly simpler so that we don't have to do a much more comprehensive refactor of this code up front.

suddjian · 2020-10-17T00:01:05Z

packages/superset-ui-core/src/query/types/Query.ts

+  | '__granularity';
+
+export type QueryObjectAppliedTimeExtras = {
+  QueryObjectAppliedTimeExtraKey?: string;


I don't think this type is doing what you think it's doing

suddjian · 2020-10-17T00:04:39Z

I'm not sure what applied_extra_filters is meant to be used for. Is it a mapping to column names?

villebro · 2020-10-19T10:56:24Z

packages/superset-ui-core/src/query/extractExtras.ts

+  const { extras = {}, filters = [] } = formData;
  const partialQueryObject: Partial<QueryObject> = {
-    filters: formData.filters || [],
-    extras: formData.extras || {},
+    filters,
+    extras,
+    applied_time_extras,


Bycatch: change to use destructuring here.

villebro · 2020-10-19T11:03:08Z

packages/superset-ui-core/src/query/extractExtras.ts

-    partialQueryObject.extras = {
-      druid_time_origin: formData.druid_time_origin,
-    };
+    extras.druid_time_origin = formData.druid_time_origin;


bycatch: better to add to the object than discard the original one.

villebro · 2020-10-19T11:03:45Z

packages/superset-ui-core/src/query/extractExtras.ts

-    partialQueryObject.extras = {
-      ...partialQueryObject.extras,
-      time_grain_sqla: partialQueryObject.time_grain_sqla || formData.time_grain_sqla,
-    };
+    extras.time_grain_sqla = partialQueryObject.time_grain_sqla || formData.time_grain_sqla;


villebro · 2020-10-19T11:06:49Z

@suddjian and @ktmud thanks for your comments, I did my best to address them. I added a link to the incubator-superset PR, which implements these changes for the new chart data endpoint (also implements the same for the legacy endpoint).

suddjian

lgtm

ktmud

LGTM

feat(core): add applied_time_extras to QueryObject

48153fb

villebro requested a review from suddjian October 16, 2020 13:46

villebro requested a review from a team as a code owner October 16, 2020 13:46

pull-request-size bot added the size/S label Oct 16, 2020

vercel bot deployed to Preview October 16, 2020 13:46 View deployment

pkdotson approved these changes Oct 16, 2020

View reviewed changes

ktmud reviewed Oct 16, 2020

View reviewed changes

suddjian reviewed Oct 17, 2020

View reviewed changes

villebro mentioned this pull request Oct 19, 2020

fix(extra-filters): add logic for identifying applied extra filters apache/superset#11325

Merged

6 tasks

pull-request-size bot added size/M and removed size/S labels Oct 19, 2020

vercel bot deployed to Preview October 19, 2020 08:51 View deployment

villebro commented Oct 19, 2020

View reviewed changes

address review comments

4127393

villebro force-pushed the villebro/applied-time-extras branch from 12b97bb to 4127393 Compare October 19, 2020 10:59

vercel bot deployed to Preview October 19, 2020 10:59 View deployment

villebro commented Oct 19, 2020

View reviewed changes

suddjian approved these changes Oct 19, 2020

View reviewed changes

ktmud approved these changes Oct 19, 2020

View reviewed changes

villebro merged commit fbea70c into apache-superset:master Oct 19, 2020

villebro deleted the villebro/applied-time-extras branch October 19, 2020 17:39

Conversation

villebro commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pkdotson left a comment

Choose a reason for hiding this comment

Uh oh!

villebro commented Oct 16, 2020

Uh oh!

ktmud left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

villebro commented Oct 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suddjian commented Oct 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

villebro commented Oct 19, 2020

Uh oh!

suddjian left a comment

Choose a reason for hiding this comment

Uh oh!

ktmud left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

villebro commented Oct 16, 2020 •

edited

Loading

vercel bot commented Oct 16, 2020 •

edited

Loading

codecov bot commented Oct 16, 2020 •

edited

Loading