[Explore] Adding Adhoc Filters #4909

gabe-lyons · 2018-04-30T18:11:15Z

Here I'm following a similar design pattern as Adhoc Metrics (#4663 and #4736). Adhoc Filters introduces a new control that unifies the 4 previous filter controls (free text where, free text having, structured where filter, structured having filter). It allows you to select a column or any saved metric / adhoc metric currently being used in the metric field to filter by:

and then once a metric or column is selected lets you switch between writing a SQL filter or simple filter for that metric/column:

(for druid datsources, metric and column filters can only be edited in the simple view. for table datsources, column filters can be edited in either but metrics filters can only be edited in the sql view)

This PR also removes the legacy filter controls. When loading a chart that has legacy filters already attached to it, they will be cast to adhoc filters:

Here are some more demos of the feature in action,

creating and editing filters:

transitioning between simple tab and sql tab:

selecting adhoc metrics to filter on:

notes to reviewers:
The meat of the frontend lies in the three AdhocFilterEditPopover* files and the AdhocFilterControl file. The meat of the backend lies in viz.py where I break adhoc filters out into the four base types of filters for processing.

reviewers:
@michellethomas @williaster @graceguo-supercat @john-bodley @mistercrunch

codecov-io · 2018-04-30T20:13:30Z

Codecov Report

Merging #4909 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4909      +/-   ##
==========================================
+ Coverage   77.07%   77.15%   +0.07%     
==========================================
  Files          44       44              
  Lines        8558     8587      +29     
==========================================
+ Hits         6596     6625      +29     
  Misses       1962     1962

Impacted Files	Coverage Δ
superset/connectors/druid/models.py	`81.11% <100%> (-0.06%)`	⬇️
superset/viz.py	`80.29% <100%> (+0.39%)`	⬆️
superset/utils.py	`87.9% <100%> (+0.05%)`	⬆️

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 f21ba1a...e3ec913. Read the comment docs.

gabe-lyons · 2018-05-04T22:20:14Z

Made a couple of aesthetic changes based on feedback from the team

Pills now appear darker and more clickable:

the text area is also wrapped in a thin border (and by wraps overflow text to new lines rather than hiding it):

I also introduced some ways to give feedback that simple filters cannot filter on metrics in table datasources:

williaster

this looks good to me overall! 💯 a couple small comments :)

williaster · 2018-05-07T17:06:12Z

superset/assets/spec/javascripts/explore/AdhocFilter_spec.js

+      comparator: '10',
+      clause: CLAUSES.WHERE,
+    });
+    expect(adhocFilter.filterOptionName.length).to.be.above(10);


is this because value > 10 has a length of 10? you could maybe make it more readable/obvious by doing 'value > 10'.length

ah the optionNames are just randomized ids in this case. we use them to differentiate options when updating (sort of like a react key). so I can't predict what its value is.

williaster · 2018-05-07T17:12:41Z

superset/assets/src/explore/AdhocFilter.js

+
+  duplicateWith(nextFields) {
+    return new AdhocFilter({
+      ...this,


not a blocker, but I think in general it's better for readability and bugginess to explicitly define which properties are copied (for ...this, I think ...nextFields is fine)

williaster · 2018-05-07T17:15:29Z

superset/assets/src/explore/AdhocFilter.js

+function translateToSql(adhocMetric, { useSimple } = {}) {
+  if (adhocMetric.expressionType === EXPRESSION_TYPES.SIMPLE || useSimple) {
+    const isMulti = MULTI_OPERATORS.indexOf(adhocMetric.operator) >= 0;
+    return `${adhocMetric.subject} ${OPERATORS_TO_SQL[adhocMetric.operator]} ${isMulti ? '(\'' : ''}${isMulti ? adhocMetric.comparator.join("','") : adhocMetric.comparator}${isMulti ? '\')' : ''}`;


you could break this up into different pieces to make it more readable.

williaster · 2018-05-07T17:16:24Z

superset/assets/src/explore/components/AdhocFilterEditPopover.jsx

+  onChange: PropTypes.func.isRequired,
+  onClose: PropTypes.func.isRequired,
+  onResize: PropTypes.func.isRequired,
+  options: PropTypes.arrayOf(PropTypes.oneOfType([


yay for well defined shapes! 🎉

williaster · 2018-05-07T17:17:23Z

superset/assets/src/explore/components/AdhocFilterEditPopover.jsx

+      width: startingWidth,
+      height: startingHeight,
+    };
+    document.addEventListener('mouseup', this.onMouseUp);


I've typically seen this in the componentDidMount lifecycle event but am not sure if it matters too much.

williaster · 2018-05-07T17:32:44Z

superset/assets/src/explore/components/AdhocFilterEditPopoverSimpleTabContent.jsx

+      )),
+      value: adhocFilter.operator,
+      onChange: this.onOperatorChange,
+      optionRenderer: VirtualizedRendererWrap((


so are we just doing readable options for equals? any reason not to make them all human readable?

(to clarify, I think readable labels are good for the options dropdown, I think the mathematical representation >, != etc is still probably better for the label since it needs to be concise.) not set on this though, I would imagine most people would understand either. an alternative I think we discussed would be to include both representations in the dropdown eg != (not equal to)

williaster · 2018-05-07T17:35:40Z

superset/assets/src/explore/components/AdhocFilterOption.jsx

+        onExited={this.onOverlayExited}
+      >
+        <Label className="adhoc-filter-option">
+          <div onMouseDownCapture={(e) => { e.stopPropagation(); }}>


could you define this as a class function so it's not initialized every render?

williaster · 2018-05-07T17:37:32Z

superset/assets/src/explore/components/controls/AdhocFilterControl.jsx

+  val: PropTypes.oneOfType([PropTypes.string, PropTypes.arrayOf(PropTypes.string)]),
+});
+
+const propTypes = {


typically you should define default props for anything not required because it's less likely you'll throw an error.

williaster · 2018-05-07T17:42:17Z

superset/assets/src/explore/components/controls/SelectControl.jsx

@@ -43,6 +44,7 @@ const defaultProps = {
  optionRenderer: opt => opt.label,
  valueRenderer: opt => opt.label,
  valueKey: 'value',
+  noResultsText: 'No results found',


should this be a t()?

williaster · 2018-05-07T17:43:03Z

superset/assets/src/explore/main.css

+}
+
+.adhoc-filter-edit-tabs > .nav-tabs > li > a {
+  padding: 4px 4px 4px 4px;


you might know this but padding: 4px also works

michellethomas · 2018-05-07T19:14:59Z

superset/assets/src/visTypesOverride.js

@@ -0,0 +1,5 @@
+import visTypes from './explore/visTypes';


Did you mean to commit this?

john-bodley · 2018-05-07T22:24:15Z

superset/viz.py

+                            'op': adhoc_filter.get('operator'),
+                            'val': adhoc_filter.get('comparator'),
+                        })
+                if expression_type == 'SQL':


Nit. s/if/elif.

john-bodley · 2018-05-07T22:24:22Z

superset/viz.py

+                            'op': adhoc_filter.get('operator'),
+                            'val': adhoc_filter.get('comparator'),
+                        })
+                    if clause == 'HAVING':


Nit. s/if/elif.

john-bodley · 2018-05-07T22:24:52Z

superset/viz.py

+                if expression_type == 'SQL':
+                    if clause == 'WHERE':
+                        sql_where_filters.append(adhoc_filter.get('sqlExpression'))
+                    if clause == 'HAVING':


Nit. s/if/elif.

john-bodley · 2018-05-07T22:27:39Z

superset/viz.py

+                        sql_having_filters.append(adhoc_filter.get('sqlExpression'))
+            all_where_text = ''
+            all_having_text = ''
+            if len(sql_where_filters) == 1:


Why not simply,

all_where_text = ' AND '.join(['({})'.format(sql) for sql in sql_where_filters)])

Note list comprehensions are preferred over using map, filter etc.

ah nice thats so much shorter 🎉

john-bodley · 2018-05-07T22:28:54Z

superset/viz.py

+                all_where_text = ' AND '.join(list(map(
+                    lambda expression: '({})'.format(expression), sql_where_filters,
+                )))
+            if len(sql_having_filters) == 1:


gabe-lyons · 2018-05-08T00:03:19Z

@williaster @michellethomas @john-bodley comments addressed, PTAL a when you have the chance!

john-bodley · 2018-05-08T02:48:52Z

superset/viz.py

+            extras = {
+                'where': ' AND '.join(['({})'.format(sql) for sql in sql_where_filters]),
+                'having': ' AND '.join(
+                    ['({})'.format(sql) for sql in sql_having_filters],


Does this go over the 90 character limit without wrapping?

by one character 😢

graceguo-supercat · 2018-05-10T17:41:47Z

🔥 🎆 🍾 🎊

* adding custom expressions to adhoc metrics * adjusted transitions and made the box expandable * adding adhoc filters * adjusted based on feedback

jasnovak · 2018-06-13T23:33:00Z

@GabeLoins @john-bodley This is causing a bug with filters not being saved when visualizations are changed. specifically when switching to Table View: #5172

* adding custom expressions to adhoc metrics * adjusted transitions and made the box expandable * adding adhoc filters * adjusted based on feedback

GabeLoins added 2 commits April 24, 2018 16:08

adding custom expressions to adhoc metrics

67dcc91

adjusted transitions and made the box expandable

1b3c568

gabe-lyons force-pushed the gabe_adding_adhoc_filters branch 3 times, most recently from fac4cf6 to c85c7f1 Compare April 30, 2018 19:34

adding adhoc filters

c85c7f1

williaster reviewed May 7, 2018

View reviewed changes

michellethomas reviewed May 7, 2018

View reviewed changes

john-bodley reviewed May 7, 2018

View reviewed changes

gabe-lyons force-pushed the gabe_adding_adhoc_filters branch from d3c99d4 to 6e84b49 Compare May 8, 2018 00:02

gabe-lyons force-pushed the gabe_adding_adhoc_filters branch 4 times, most recently from 88e31b7 to e3ec913 Compare May 8, 2018 01:13

adjusted based on feedback

e3ec913

john-bodley reviewed May 8, 2018

View reviewed changes

graceguo-supercat merged commit a8514b2 into apache:master May 10, 2018

jasnovak mentioned this pull request Jun 12, 2018

Filters not saved when visualization changed #5172

Closed

ktmud mentioned this pull request Feb 20, 2021

[Explore][Control] Allow selecting ad-hoc metrics in filters #13249

Closed

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.26.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Explore] Adding Adhoc Filters #4909

[Explore] Adding Adhoc Filters #4909

gabe-lyons commented Apr 30, 2018

codecov-io commented Apr 30, 2018 •

edited

Loading

gabe-lyons commented May 4, 2018

williaster left a comment

williaster May 7, 2018

gabe-lyons May 7, 2018

williaster May 7, 2018

williaster May 7, 2018

williaster May 7, 2018

williaster May 7, 2018

williaster May 7, 2018

williaster May 7, 2018

williaster May 7, 2018

williaster May 7, 2018

williaster May 7, 2018

williaster May 7, 2018

michellethomas May 7, 2018

gabe-lyons May 7, 2018

john-bodley May 7, 2018

john-bodley May 7, 2018

john-bodley May 7, 2018

john-bodley May 7, 2018

gabe-lyons May 7, 2018

john-bodley May 7, 2018

gabe-lyons commented May 8, 2018

john-bodley May 8, 2018

gabe-lyons May 8, 2018

graceguo-supercat commented May 10, 2018

jasnovak commented Jun 13, 2018 •

edited

Loading

[Explore] Adding Adhoc Filters #4909

[Explore] Adding Adhoc Filters #4909

Conversation

gabe-lyons commented Apr 30, 2018

codecov-io commented Apr 30, 2018 • edited Loading

Codecov Report

gabe-lyons commented May 4, 2018

williaster left a 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

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

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

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

gabe-lyons commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat commented May 10, 2018

jasnovak commented Jun 13, 2018 • edited Loading

codecov-io commented Apr 30, 2018 •

edited

Loading

jasnovak commented Jun 13, 2018 •

edited

Loading