feat: update ChartFormData and QueryObject to support filters. #164

kristw · 2019-05-22T19:15:52Z

💔 Breaking Changes

Rename types to make it clearer. The metric types in ChartFormData and QueryObject are different.

FormDataMetric => ChartFormDataMetric
Metrics => ChartFormDataMetrics
Metric => QueryObjectMetric

This is pretty mild breaking change as these are internally used types so I would incline to not bump major version.

🏆 Enhancements

Update type of ChartFormData and QueryObject to support filters.
Update buildQueryObject to convert filters from ChartFormData and put into corresponding fields in QueryObject. See processFilters.ts and convertFilter.ts.
Improve typings of the QueryObject fields.
Add type guards

📜 Documentation

Add comments for fields on QueryObject

🏠 Internal

Remove Metrics class and replace with two functions processMetrics and convertMetric.
Migrate old unit tests and add new unit tests to ensure coverage.

codecov · 2019-05-22T20:53:59Z

Codecov Report

Merging #164 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #164   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          81     88    +7     
  Lines        1050   1095   +45     
  Branches      259    263    +4     
=====================================
+ Hits         1050   1095   +45

Impacted Files	Coverage Δ
...es/superset-ui-chart/src/query/buildQueryObject.ts	`100% <100%> (ø)`	⬆️
...kages/superset-ui-chart/src/query/processExtras.ts	`100% <100%> (ø)`
...ages/superset-ui-chart/src/query/processFilters.ts	`100% <100%> (ø)`
...ages/superset-ui-chart/src/query/processMetrics.ts	`100% <100%> (ø)`
...kages/superset-ui-chart/src/query/convertMetric.ts	`100% <100%> (ø)`
packages/superset-ui-chart/src/types/Operator.ts	`100% <100%> (ø)`
packages/superset-ui-chart/src/types/Filter.ts	`100% <100%> (ø)`
...kages/superset-ui-chart/src/query/convertFilter.ts	`100% <100%> (ø)`
...kages/superset-ui-chart/src/types/ChartFormData.ts	`100% <100%> (ø)`
... and 7 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 6c732ec...f702ca1. Read the comment docs.

netlify · 2019-05-22T21:35:09Z

Deploy preview for superset-ui ready!

Built with commit 1e8853e

https://deploy-preview-164--superset-ui.netlify.com

conglei · 2019-05-23T05:46:25Z

packages/superset-ui-chart/src/types/Query.ts

  row_limit?: number;
+  /** Maximum number of series */
+  timeseries_limit?: number;
+  /** TODO: Doc */


The metric used to sort the returned result.

conglei · 2019-05-23T06:26:27Z

packages/superset-ui-chart/src/query/convertFilter.ts

+export default function convertFilter(filter: SimpleAdhocFilter): QueryObjectFilterClause {
+  const { subject } = filter;
+  if (isUnaryAdhocFilter(filter)) {
+    const { operator } = filter;


We can put it outside the current block to reduce the duplication.

Has to be inside the block to infer the correct Operator type

conglei · 2019-05-23T14:33:24Z

packages/superset-ui-chart/src/types/Operator.ts

@@ -0,0 +1,43 @@
+/** List of operators that do not require another operand */
+export const UNARY_OPERATORS = ['IS NOT NULL', 'IS NULL'] as const;


do we need to export UNARY_OPERATORS etc? seems there is no other place to use them.

Not being used at the moment. I will remove export

conglei · 2019-05-23T14:35:51Z

packages/superset-ui-chart/src/query/processFilters.ts

+  // TODO: Implement
+  // utils.convert_legacy_filters_into_adhoc(self.form_data)
+
+  // TODO: Implement


Will these two in the scope of this PR?

These will be beyond the scope. Right now this PR will handle adhoc_filters (which is the standard filters) correctly plus provide typings.

do you think we should console.warn if we see legacy filters? (fine if not)

The legacy filters are using fields filters and having_filters which do not exist on type ChartFormData, so typescript would complain.

I am wondering if we want to support this utils.convert_legacy_filters_into_adhoc(self.form_data) going forward. Not sure how long since we have moved from the legacy filters.

conglei

Thanks for adding this and refactoring the code overall! it looks good overall

williaster · 2019-05-23T17:45:28Z

packages/superset-ui-chart/src/query/buildQueryObject.ts

-    order_desc: orderDesc,
+    is_timeseries: groupbySet.has(DTTM_ALIAS),
+    metrics: processMetrics(formData),
+    order_desc: order_desc === undefined ? true : order_desc,


typeof order_desc === 'undefined'?

williaster · 2019-05-23T18:06:40Z

packages/superset-ui-chart/src/types/Operator.ts

@@ -0,0 +1,43 @@
+/** List of operators that do not require another operand */


wow these are great!

williaster · 2019-05-23T18:12:02Z

packages/superset-ui-chart/src/query/processFilters.ts

+  // TODO: Implement
+  // merge_extra_filters(self.form_data)
+
+  // utils.split_adhoc_filters_into_base_filters(self.form_data)


is this just indicating what the logic below mapped to? (i.e., it's not part of the TODOs above) should we remove it or keep for context?

Will remove or change to simple sentence. Seem not necessary any more

williaster · 2019-05-23T18:14:56Z

packages/superset-ui-chart/src/query/processFilters.ts

+  // eslint-disable-next-line camelcase
+  const { adhoc_filters } = formData;
+  if (Array.isArray(adhoc_filters)) {
+    const simpleWhere: QueryObjectFilterClause[] = [];


(not blocking) the type helps but I find these a little confusing name-wise, not sure what to expect simple to mean, maybe whereFilter <> whereFreeform and havingFilter <> havingFreeform?

williaster · 2019-05-23T18:17:25Z

packages/superset-ui-chart/src/types/Metric.ts

-  SIMPLE = 'SIMPLE',
-  SQL = 'SQL',
-}
+export type Aggregate = 'AVG' | 'COUNT' | 'COUNT_DISTINCT' | 'MAX' | 'MIN' | 'SUM';


williaster

really solid! Hard to find anything to improve significantly I had only nit/non-blocking thoughts 😍 👏 💯

Updates the requirements on [@data-ui/event-flow](https://github.com/williaster/data-ui) to permit the latest version. - [Release notes](https://github.com/williaster/data-ui/releases) - [Changelog](https://github.com/williaster/data-ui/blob/master/CHANGELOG.md) - [Commits](williaster/data-ui@v0.0.79...v0.0.80) Signed-off-by: dependabot-preview[bot] <[email protected]>

kristw added 2 commits May 21, 2019 22:43

feat: update types to include filters

e349fa7

refactor: remove namespace

0ec5b5a

kristw requested a review from a team as a code owner May 22, 2019 19:15

refactor: split function

496e202

pull-request-size bot added the size/L label May 22, 2019

kristw added 5 commits May 22, 2019 16:01

feat: add filter processing and refactor

07435ee

feat: revise metrics processing

1b4392e

test: add unit tests

711acfe

refactor: move type files back

648dc8d

test: add unit tests

d1a4ab7

pull-request-size bot added size/XXL and removed size/L labels May 23, 2019

fix: unit test

1e8853e

kristw changed the title ~~[WIP] feat: add filter~~ feat: update ChartFormData and QueryObject to support filters. May 23, 2019

kristw requested review from conglei and xtinec May 23, 2019 00:54

conglei reviewed May 23, 2019

View reviewed changes

conglei approved these changes May 23, 2019

View reviewed changes

kristw added 2 commits May 23, 2019 09:19

fix: remove exports

bb1d5b5

docs: add field info

e9ac969

williaster reviewed May 23, 2019

View reviewed changes

fix: type check undefined

3bbe164

williaster reviewed May 23, 2019

View reviewed changes

williaster approved these changes May 23, 2019

View reviewed changes

kristw added 2 commits May 23, 2019 12:06

docs: add more comments

a005c71

build: speed up storybook build

f702ca1

kristw merged commit 025d5bc into master May 23, 2019

delete-merged-branch bot deleted the kristw--filter branch May 23, 2019 20:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update ChartFormData and QueryObject to support filters. #164

feat: update ChartFormData and QueryObject to support filters. #164

kristw commented May 22, 2019 •

edited

Loading

codecov bot commented May 22, 2019 •

edited

Loading

netlify bot commented May 22, 2019 •

edited

Loading

conglei May 23, 2019

conglei May 23, 2019

kristw May 23, 2019

conglei May 23, 2019

kristw May 23, 2019

conglei May 23, 2019

kristw May 23, 2019 •

edited

Loading

williaster May 23, 2019

kristw May 23, 2019

conglei left a comment

williaster May 23, 2019 •

edited

Loading

williaster May 23, 2019

williaster May 23, 2019

kristw May 23, 2019

williaster May 23, 2019

williaster May 23, 2019

williaster left a comment

		@@ -0,0 +1,43 @@
		/** List of operators that do not require another operand */
		export const UNARY_OPERATORS = ['IS NOT NULL', 'IS NULL'] as const;

feat: update ChartFormData and QueryObject to support filters. #164

feat: update ChartFormData and QueryObject to support filters. #164

Conversation

kristw commented May 22, 2019 • edited Loading

codecov bot commented May 22, 2019 • edited Loading

Codecov Report

netlify bot commented May 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristw May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conglei left a comment

Choose a reason for hiding this comment

williaster May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williaster left a comment

Choose a reason for hiding this comment

kristw commented May 22, 2019 •

edited

Loading

codecov bot commented May 22, 2019 •

edited

Loading

netlify bot commented May 22, 2019 •

edited

Loading

kristw May 23, 2019 •

edited

Loading

williaster May 23, 2019 •

edited

Loading