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

fix: implement extra filter logic#688

Merged
villebro merged 6 commits intoapache-superset:masterfrom
preset-io:villebro/fix-extra-filters
Jul 21, 2020
Merged

fix: implement extra filter logic#688
villebro merged 6 commits intoapache-superset:masterfrom
preset-io:villebro/fix-extra-filters

Conversation

@villebro
Copy link
Copy Markdown
Contributor

@villebro villebro commented Jul 17, 2020

🐛 Bug Fix

This implements the merge_extra_filters function + applicable features other legacy Python code in utils/core.py to fix these TODOs:

/** Logic formerly in viz.py's process_query_filters */
export default function processFilters(formData: QueryFormData) {
  // TODO: Implement
  // utils.convert_legacy_filters_into_adhoc(self.form_data)

  // TODO: Implement
  // merge_extra_filters(self.form_data)

This removes all deprecated QueryObject fields (for instance, replacing QueryObject.granularity_sqla with granularity and QueryObject.having to QueryObject.extras.having etc) and makes extra filters work properly.

@villebro villebro requested a review from a team as a code owner July 17, 2020 15:33
@vercel
Copy link
Copy Markdown

vercel bot commented Jul 17, 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/h4r3c3xon
✅ Preview: https://superset-ui-git-fork-preset-io-villebro-fix-extra-filters.superset.vercel.app

@villebro villebro changed the title [WIP] fix: implement extra filter logic fix: implement extra filter logic Jul 20, 2020
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 20, 2020

Codecov Report

Merging #688 into master will increase coverage by 0.15%.
The diff coverage is 96.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #688      +/-   ##
==========================================
+ Coverage   24.04%   24.20%   +0.15%     
==========================================
  Files         338      338              
  Lines        7590     7607      +17     
  Branches      918      924       +6     
==========================================
+ Hits         1825     1841      +16     
  Misses       5692     5692              
- Partials       73       74       +1     
Impacted Files Coverage Δ
...kages/superset-ui-query/src/types/QueryFormData.ts 100.00% <ø> (ø)
packages/superset-ui-query/src/processFilters.ts 95.83% <80.00%> (-4.17%) ⬇️
packages/superset-ui-query/src/buildQueryObject.ts 100.00% <100.00%> (ø)
packages/superset-ui-query/src/extractExtras.ts 100.00% <100.00%> (ø)
...s/plugin-chart-word-cloud/src/plugin/buildQuery.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 1d700aa...4ec35e9. Read the comment docs.

// map to undeprecated names and remove deprecated fields
if (isDruidFormData(formData) && !partialQueryObject.druid_time_origin) {
partialQueryObject.extras = {
druid_time_origin: formData.druid_time_origin,
Copy link
Copy Markdown
Member

@rusackas rusackas Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

druid_time_origin is optional on the data type checked by isDruidFormData. Should a default be specified here, e.g. druid_time_origin: formData.druid_time_origin || someDefault,?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backend expects most of these to be undefined if they haven't been given an explicit value (time_grain, druid_time_origin etc). So these should be safe to leave as is.

Copy link
Copy Markdown
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but want to make sure that extractExtras.ts handles undefined parameters and/or provides default values wherever needed, just to be safe.

@villebro villebro merged commit b24615e into apache-superset:master Jul 21, 2020
@villebro villebro deleted the villebro/fix-extra-filters branch July 21, 2020 03:24
simcha90 pushed a commit to simcha90/superset-ui that referenced this pull request Jul 26, 2020
* fix: implement extra filter logic

* fix bugs and add tests

* remove redundant changes

* improve types

* fix coverage

* improve codevov
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.

2 participants