feat(core): add extra form data fields for native filters by villebro · Pull Request #992 · apache-superset/superset-ui

villebro · 2021-03-05T15:47:49Z

🏆 Enhancements

List of changes:

add placeholder props time_grain and time_column fields to QueryFormData to get ready to streamline granularity, granularity_sqla and time_grain_sqla.
fix appending of filters and adhoc filters + add tests
remove appendExtraFormData, as we should only enable appending filters (are handled by processFilters.ts now to centralize filter handling in buildQueryObject)
add new override props + add tests
fix a few typos in buildQueryContext.ts

vercel · 2021-03-05T15:47:55Z

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/DwsZGbJXg2xVALdsEvXbQYxirjee
✅ Preview: https://superset-ui-git-fork-preset-io-villebro-new-query-par-bfbe07.vercel.app

codecov · 2021-03-05T16:09:55Z

Codecov Report

Merging #992 (bd02652) into master (deeee7c) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #992      +/-   ##
==========================================
- Coverage   27.94%   27.88%   -0.07%     
==========================================
  Files         413      413              
  Lines        8523     8515       -8     
  Branches     1204     1201       -3     
==========================================
- Hits         2382     2374       -8     
  Misses       5987     5987              
  Partials      154      154

Impacted Files	Coverage Δ
...kages/superset-ui-core/src/query/processFilters.ts	`100.00% <ø> (ø)`
...ges/superset-ui-core/src/query/buildQueryObject.ts	`100.00% <100.00%> (ø)`
...ckages/superset-ui-core/src/query/extractExtras.ts	`100.00% <100.00%> (ø)`
...superset-ui-core/src/query/processExtraFormData.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 deeee7c...bd02652. Read the comment docs.

ktmud · 2021-03-05T19:56:23Z

add placeholder props time_grain and time_column fields to QueryFormData to get ready to streamline granularity, granularity_sqla and time_grain_sqla.

This is long overdue! I still have a hard time wrapping my head around every time I see granularity.

simcha90 · 2021-03-07T09:36:48Z

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

+    adhoc_filters: AdhocFilter[];
+  } = {
+    filters: [...(formData.filters || []), ...(append_form_data.filters || [])],
+    adhoc_filters: [...(formData.adhoc_filters || []), ...(append_form_data.adhoc_filters || [])],


I think better use ??... to do all like:

...(formData.filters ?? [])

filters: [].concat(extras.filters, append_form_data.filters).filter(Boolean)

I think this would be both faster and safer?

simcha90 · 2021-03-07T09:44:21Z

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


  const extras = extractExtras(formData);
+  // collect all filters for conversion to simple filters/freeform clauses
+  const filterFormData: {


2 questions:

Should this functionality to be in extractExtras?

Should we add here also filters and adhoc_filters from custom_form_data and override_form_data?

extractExtras and processFilters probably need to be slightly refactored now that there's more stuff going on here, but since this code is slightly convoluted I prefer to keep these changes as small as possible for now. Let's do a proper refactor soon when we know the full scope of changes required by native and cross filters.

I don't think we should to allow overriding filters, as it could break charts, but happy to open up this discussion if there are known use cases for it. Also, custom_form_data probably shouldn't include filters and adhoc_filters, as those are already defined in QueryFormData and QueryObject.

About 2. For example what will be behavior for a case:
I have table with server pagination and cross filtering:

User clicks Page 2 we have in custom_form_data data about server pagination

We have button in dashboards like resetDashboard so we need reset it to Page 1 or stuff like that

simcha90

approve, but some questions left

ktmud

LGTM with some questions. Looking forward to further cleanups in this area.

ktmud · 2021-03-07T20:25:14Z

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

+    adhoc_filters: AdhocFilter[];
+  } = {
+    filters: [...(formData.filters || []), ...(append_form_data.filters || [])],
+    adhoc_filters: [...(formData.adhoc_filters || []), ...(append_form_data.adhoc_filters || [])],


filters: [].concat(extras.filters, append_form_data.filters).filter(Boolean)

I think this would be both faster and safer?

ktmud · 2021-03-07T21:01:13Z

packages/superset-ui-core/test/query/buildQueryObject.test.ts

+              sqlExpression: '(1 = 1)',
+            },
+          ],
+        },


Question about adhoc_filters and filters. Is my understanding correct that filters are only used for native filters and can only be set from append_form_data, since all filters defined in chart formData are basically adhoc_filters?

I'm a little confused why do we need to allow appending and overriding both filters and adhoc_filters. The overrides are only from a Dashboard's native/cross filters, which would be only in QueryObjectFilterClause or QueryFormExtraFilter format.

It seems to me the original design of an adhoc_filters vs extra_filters was cleaner.

Not sure if this is the direction of future refactoring, but we probably shouldn't add extra stuff that are not configurable by chart controls to formData. This extra_form_data could be moved to the extra options of buildQueryObject and buildQuery. E.g.

buildQueryObject(formData, { queryFields: ... // configured in chart metatada, extraFormData: { append: { adhoc_filter: [...], }, override: { } } // collected from dashboard state });

AFAIK filters are just the simple filters that preceeded adhoc_filters (before my time). I'm not sure why we support both, as adhoc_filters are a superset of filters, but my assumption is it's a leftover from old times. I've been using the filters property when adding filters that are simple, i.e. don't require custom SQL, but we could equally well ditch filters and just make it possible append to adhoc_filters.

We should probably:

Rename the Python attribute query_object.filter to query_object.filters

Rename adhoc_filters on the client side to filters and have it support both simple filter and adhoc filters.

Move the filter format consolidation logic to the server side. Either in QueryObject itself or when actually building the SQL queries.

…erset#992) * feat(core): implement time grain and column filters * add apply_fetch_values_predicate to QueryObject * clean up extract types and fix tests

villebro requested a review from a team as a code owner March 5, 2021 15:47

pull-request-size bot added the size/L label Mar 5, 2021

vercel bot had a problem deploying to Preview March 5, 2021 15:47 Failure

villebro force-pushed the villebro/new-query-params branch from c3ad624 to 483b1e2 Compare March 5, 2021 15:59

vercel bot deployed to Preview March 5, 2021 15:59 View deployment

villebro mentioned this pull request Mar 5, 2021

feat(native-filters): add timegrain and column filter apache/superset#13484

Merged

6 tasks

vercel bot deployed to Preview March 5, 2021 21:52 View deployment

villebro changed the title ~~[WIP] feat(core): add extra form data fields for native filters~~ feat(core): add extra form data fields for native filters Mar 7, 2021

simcha90 reviewed Mar 7, 2021

View reviewed changes

simcha90 approved these changes Mar 7, 2021

View reviewed changes

villebro force-pushed the villebro/new-query-params branch from 05d6541 to 8c46beb Compare March 7, 2021 12:42

vercel bot deployed to Preview March 7, 2021 12:42 View deployment

ktmud approved these changes Mar 7, 2021

View reviewed changes

feat(core): implement time grain and column filters

8239476

villebro force-pushed the villebro/new-query-params branch from 8c46beb to cfe6de8 Compare March 8, 2021 11:45

vercel bot deployed to Preview March 8, 2021 11:45 View deployment

add apply_fetch_values_predicate to QueryObject

555dd3b

villebro force-pushed the villebro/new-query-params branch from cfe6de8 to 555dd3b Compare March 8, 2021 12:18

vercel bot deployed to Preview March 8, 2021 12:18 View deployment

vercel bot deployed to Preview March 8, 2021 15:38 View deployment

villebro force-pushed the villebro/new-query-params branch from 4294d4f to f01b2ae Compare March 8, 2021 16:21

vercel bot deployed to Preview March 8, 2021 16:21 View deployment

vercel bot deployed to Preview March 8, 2021 17:06 View deployment

villebro force-pushed the villebro/new-query-params branch from e6518b3 to fba64d3 Compare March 8, 2021 19:03

vercel bot had a problem deploying to Preview March 8, 2021 19:03 Failure

clean up extract types and fix tests

bd02652

villebro force-pushed the villebro/new-query-params branch from fba64d3 to bd02652 Compare March 8, 2021 19:09

vercel bot deployed to Preview March 8, 2021 19:09 View deployment

villebro merged commit a2f0aff into apache-superset:master Mar 8, 2021

villebro deleted the villebro/new-query-params branch March 8, 2021 20:51

Comments

Conversation

villebro commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ktmud commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

simcha90 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

villebro commented Mar 5, 2021 •

edited

Loading

vercel bot commented Mar 5, 2021 •

edited

Loading

codecov bot commented Mar 5, 2021 •

edited

Loading

ktmud commented Mar 5, 2021 •

edited

Loading