fixes for bugs in #3689 (#3692)

superset/assets/javascripts/explore/components/controls/Filter.jsx

@@ -109,7 +109,7 @@ export default class Filter extends React.Component {
       <input
         type="text"
         onChange={this.changeText.bind(this)}
-        value={filter.val}
+        value={filter.val || ''}
         className="form-control input-sm"
         placeholder={t('Filter value')}
       />

superset/assets/spec/javascripts/explore/components/Filter_spec.jsx

@@ -88,4 +88,28 @@ describe('Filter', () => {
     const regexWrapper = shallow(<Filter {...props} />);
     expect(regexWrapper.find('input')).to.have.lengthOf(1);
   });
+
+  it('renders `input` for text filters', () => {
+    const props = Object.assign({}, defaultProps);
+    ['>=', '>', '<=', '<'].forEach((op) => {
+      props.filter = {
+        col: 'col1',
+        op,
+        value: 'val',
+      };
+      wrapper = shallow(<Filter {...props} />);
+      expect(wrapper.find('input')).to.have.lengthOf(1);
+    });
+  });
+
+  it('replaces null filter values with empty string in `input`', () => {
+    const props = Object.assign({}, defaultProps);
+    props.filter = {
+      col: 'col1',
+      op: '>=',
+      value: null,
+    };
+    wrapper = shallow(<Filter {...props} />);
+    expect(wrapper.find('input').props().value).to.equal('');
+  });
 });

superset/connectors/druid/models.py

@@ -18,7 +18,7 @@
 
 from pydruid.client import PyDruid
 from pydruid.utils.aggregators import count
-from pydruid.utils.filters import Dimension, Filter
+from pydruid.utils.filters import Dimension, Filter, Bound
 from pydruid.utils.postaggregator import (
     Postaggregator, Quantile, Quantiles, Field, Const, HyperUniqueCardinality,
 )
@@ -966,7 +966,7 @@ def run_query(  # noqa / druid
             intervals=from_dttm.isoformat() + '/' + to_dttm.isoformat(),
         )
 
-        filters = self.get_filters(filter)
+        filters = DruidDatasource.get_filters(filter, self.num_cols)
         if filters:
             qry['filter'] = filters
 
@@ -1103,11 +1103,13 @@ def increment_timestamp(ts):
             query=query_str,
             duration=datetime.now() - qry_start_dttm)
 
-    def get_filters(self, raw_filters):  # noqa
+    @staticmethod
+    def get_filters(raw_filters, num_cols):  # noqa
         filters = None
         for flt in raw_filters:
             if not all(f in flt for f in ['col', 'op', 'val']):
                 continue
+
             col = flt['col']
             op = flt['op']
             eq = flt['val']
@@ -1119,43 +1121,60 @@ def get_filters(self, raw_filters):  # noqa
                     else types
                     for types in eq]
             elif not isinstance(flt['val'], string_types):
-                eq = eq[0] if len(eq) > 0 else ''
-            if col in self.num_cols:
+                eq = eq[0] if eq and len(eq) > 0 else ''
+
+            is_numeric_col = col in num_cols
+            if is_numeric_col:
                 if op in ('in', 'not in'):
                     eq = [utils.string_to_num(v) for v in eq]
                 else:
                     eq = utils.string_to_num(eq)
+
             if op == '==':
                 cond = Dimension(col) == eq
             elif op == '!=':
-                cond = ~(Dimension(col) == eq)
+                cond = Dimension(col) != eq
             elif op in ('in', 'not in'):
                 fields = []
-                if len(eq) > 1:
+
+                # ignore the filter if it has no value
+                if not len(eq):
+                    continue
+                elif len(eq) == 1:
+                    cond = Dimension(col) == eq[0]
+                else:
                     for s in eq:
                         fields.append(Dimension(col) == s)
                     cond = Filter(type="or", fields=fields)
-                elif len(eq) == 1:
-                    cond = Dimension(col) == eq[0]
+
                 if op == 'not in':
                     cond = ~cond
+
             elif op == 'regex':
                 cond = Filter(type="regex", pattern=eq, dimension=col)
             elif op == '>=':
-                cond = Dimension(col) >= eq
+                cond = Bound(col, eq, None, alphaNumeric=is_numeric_col)
             elif op == '<=':
-                cond = Dimension(col) <= eq
+                cond = Bound(col, None, eq, alphaNumeric=is_numeric_col)
             elif op == '>':
-                cond = Dimension(col) > eq
+                cond = Bound(
+                    col, eq, None,
+                    lowerStrict=True, alphaNumeric=is_numeric_col
+                )
             elif op == '<':
-                cond = Dimension(col) < eq
+                cond = Bound(
+                    col, None, eq,
+                    upperStrict=True, alphaNumeric=is_numeric_col
+                )
+
             if filters:
                 filters = Filter(type="and", fields=[
                     cond,
                     filters
                 ])
             else:
                 filters = cond
+
         return filters
 
     def _get_having_obj(self, col, op, eq):

tests/druid_tests.py

@@ -11,7 +11,9 @@
 from mock import Mock, patch
 
 from superset import db, sm, security
-from superset.connectors.druid.models import DruidMetric, DruidCluster, DruidDatasource
+from superset.connectors.druid.models import (
+    DruidMetric, DruidCluster, DruidDatasource
+)
 from superset.connectors.druid.models import PyDruid, Quantile, Postaggregator
 
 from .base_tests import SupersetTestCase
@@ -306,11 +308,12 @@ def test_sync_druid_perm(self, PyDruid):
             metadata_last_refreshed=datetime.now())
 
         db.session.add(cluster)
-        cluster.get_datasources = PickableMock(return_value=['test_datasource'])
+        cluster.get_datasources = PickableMock(
+            return_value=['test_datasource']
+        )
         cluster.get_druid_version = PickableMock(return_value='0.9.1')
 
         cluster.refresh_datasources()
-        datasource_id = cluster.datasources[0].id
         cluster.datasources[0].merge_flag = True
         metadata = cluster.datasources[0].latest_metadata()
         self.assertEqual(len(metadata), 4)
@@ -346,12 +349,16 @@ def test_metrics_and_post_aggs(self):
                 metric_name='a_histogram',
                 verbose_name='APPROXIMATE_HISTOGRAM(*)',
                 metric_type='approxHistogramFold',
-                json=json.dumps({'type': 'approxHistogramFold', 'name': 'a_histogram'})),
+                json=json.dumps(
+                    {'type': 'approxHistogramFold', 'name': 'a_histogram'})
+                ),
             'aCustomMetric': DruidMetric(
                 metric_name='aCustomMetric',
                 verbose_name='MY_AWESOME_METRIC(*)',
                 metric_type='aCustomType',
-                json=json.dumps({'type': 'customMetric', 'name': 'aCustomMetric'})),
+                json=json.dumps(
+                    {'type': 'customMetric', 'name': 'aCustomMetric'})
+                ),
             'quantile_p95': DruidMetric(
                 metric_name='quantile_p95',
                 verbose_name='P95(*)',
@@ -396,6 +403,116 @@ def test_metrics_and_post_aggs(self):
         assert all_metrics == ['aCustomMetric']
         assert set(post_aggs.keys()) == result_postaggs
 
+    def test_get_filters_ignores_invalid_filter_objects(self):
+        filtr = {'col': 'col1', 'op': '=='}
+        filters = [filtr]
+        self.assertEqual(None, DruidDatasource.get_filters(filters, []))
+
+    def test_get_filters_constructs_filter_in(self):
+        filtr = {'col': 'A', 'op': 'in', 'val': ['a', 'b', 'c']}
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertIn('filter', res.filter)
+        self.assertIn('fields', res.filter['filter'])
+        self.assertEqual('or', res.filter['filter']['type'])
+        self.assertEqual(3, len(res.filter['filter']['fields']))
+
+    def test_get_filters_constructs_filter_not_in(self):
+        filtr = {'col': 'A', 'op': 'not in', 'val': ['a', 'b', 'c']}
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertIn('filter', res.filter)
+        self.assertIn('type', res.filter['filter'])
+        self.assertEqual('not', res.filter['filter']['type'])
+        self.assertIn('field', res.filter['filter'])
+        self.assertEqual(
+            3,
+            len(res.filter['filter']['field'].filter['filter']['fields'])
+        )
+
+    def test_get_filters_constructs_filter_equals(self):
+        filtr = {'col': 'A', 'op': '==', 'val': 'h'}
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertEqual('selector', res.filter['filter']['type'])
+        self.assertEqual('A', res.filter['filter']['dimension'])
+        self.assertEqual('h', res.filter['filter']['value'])
+
+    def test_get_filters_constructs_filter_not_equals(self):
+        filtr = {'col': 'A', 'op': '!=', 'val': 'h'}
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertEqual('not', res.filter['filter']['type'])
+        self.assertEqual(
+            'h',
+            res.filter['filter']['field'].filter['filter']['value']
+        )
+
+    def test_get_filters_constructs_bounds_filter(self):
+        filtr = {'col': 'A', 'op': '>=', 'val': 'h'}
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertFalse(res.filter['filter']['lowerStrict'])
+        self.assertEqual('A', res.filter['filter']['dimension'])
+        self.assertEqual('h', res.filter['filter']['lower'])
+        self.assertFalse(res.filter['filter']['alphaNumeric'])
+        filtr['op'] = '>'
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertTrue(res.filter['filter']['lowerStrict'])
+        filtr['op'] = '<='
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertFalse(res.filter['filter']['upperStrict'])
+        self.assertEqual('h', res.filter['filter']['upper'])
+        filtr['op'] = '<'
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertTrue(res.filter['filter']['upperStrict'])
+
+    def test_get_filters_constructs_regex_filter(self):
+        filtr = {'col': 'A', 'op': 'regex', 'val': '[abc]'}
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertEqual('regex', res.filter['filter']['type'])
+        self.assertEqual('[abc]', res.filter['filter']['pattern'])
+        self.assertEqual('A', res.filter['filter']['dimension'])
+
+    def test_get_filters_composes_multiple_filters(self):
+        filtr1 = {'col': 'A', 'op': '!=', 'val': 'y'}
+        filtr2 = {'col': 'B', 'op': 'in', 'val': ['a', 'b', 'c']}
+        res = DruidDatasource.get_filters([filtr1, filtr2], [])
+        self.assertEqual('and', res.filter['filter']['type'])
+        self.assertEqual(2, len(res.filter['filter']['fields']))
+
+    def test_get_filters_ignores_in_not_in_with_empty_value(self):
+        filtr1 = {'col': 'A', 'op': 'in', 'val': []}
+        filtr2 = {'col': 'A', 'op': 'not in', 'val': []}
+        res = DruidDatasource.get_filters([filtr1, filtr2], [])
+        self.assertEqual(None, res)
+
+    def test_get_filters_constructs_equals_for_in_not_in_single_value(self):
+        filtr = {'col': 'A', 'op': 'in', 'val': ['a']}
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertEqual('selector', res.filter['filter']['type'])
+
+    def test_get_filters_handles_arrays_for_string_types(self):
+        filtr = {'col': 'A', 'op': '==', 'val': ['a', 'b']}
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertEqual('a', res.filter['filter']['value'])
+        filtr = {'col': 'A', 'op': '==', 'val': []}
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertEqual('', res.filter['filter']['value'])
+
+    def test_get_filters_handles_none_for_string_types(self):
+        filtr = {'col': 'A', 'op': '==', 'val': None}
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertEqual('', res.filter['filter']['value'])
+
+    def test_get_filters_extracts_values_in_quotes(self):
+        filtr = {'col': 'A', 'op': 'in', 'val': ["  'a' "]}
+        res = DruidDatasource.get_filters([filtr], [])
+        self.assertEqual('a', res.filter['filter']['value'])
+
+    def test_get_filters_converts_strings_to_num(self):
+        filtr = {'col': 'A', 'op': 'in', 'val': ['6']}
+        res = DruidDatasource.get_filters([filtr], ['A'])
+        self.assertEqual(6, res.filter['filter']['value'])
+        filtr = {'col': 'A', 'op': '==', 'val': '6'}
+        res = DruidDatasource.get_filters([filtr], ['A'])
+        self.assertEqual(6, res.filter['filter']['value'])
+
 
 if __name__ == '__main__':
     unittest.main()