Filter out null locations by default (#5642)

* Filter out null locations by default

* Move exception to better place

* Add unit test

* Return columns in order for test and readibility

superset/assets/src/explore/controls.jsx

@@ -680,6 +680,13 @@ export const controls = {
     }),
   },
 
+  filter_nulls: {
+    type: 'CheckboxControl',
+    label: t('Ignore null locations'),
+    default: true,
+    description: t('Whether to ignore locations that are null'),
+  },
+
   geojson: {
     type: 'SelectControl',
     label: t('GeoJson Column'),

superset/assets/src/explore/visTypes.jsx

@@ -503,7 +503,7 @@ export const visTypes = {
         expanded: true,
         controlSetRows: [
           ['spatial', 'size'],
-          ['row_limit', null],
+          ['row_limit', 'filter_nulls'],
           ['adhoc_filters'],
         ],
       },
@@ -542,7 +542,7 @@ export const visTypes = {
         expanded: true,
         controlSetRows: [
           ['spatial', 'size'],
-          ['row_limit', null],
+          ['row_limit', 'filter_nulls'],
           ['adhoc_filters'],
         ],
       },
@@ -582,7 +582,7 @@ export const visTypes = {
         expanded: true,
         controlSetRows: [
           ['line_column', 'line_type'],
-          ['row_limit', null],
+          ['row_limit', 'filter_nulls'],
           ['adhoc_filters'],
         ],
       },
@@ -616,7 +616,7 @@ export const visTypes = {
         expanded: true,
         controlSetRows: [
           ['spatial', 'size'],
-          ['row_limit', null],
+          ['row_limit', 'filter_nulls'],
           ['adhoc_filters'],
         ],
       },
@@ -662,7 +662,8 @@ export const visTypes = {
         label: t('Query'),
         expanded: true,
         controlSetRows: [
-          ['geojson', 'row_limit'],
+          ['geojson', null],
+          ['row_limit', 'filter_nulls'],
           ['adhoc_filters'],
         ],
       },
@@ -703,7 +704,7 @@ export const visTypes = {
         expanded: true,
         controlSetRows: [
           ['line_column', 'line_type'],
-          ['row_limit', null],
+          ['row_limit', 'filter_nulls'],
           ['adhoc_filters'],
         ],
       },
@@ -744,7 +745,7 @@ export const visTypes = {
         expanded: true,
         controlSetRows: [
           ['start_spatial', 'end_spatial'],
-          ['row_limit', null],
+          ['row_limit', 'filter_nulls'],
           ['adhoc_filters'],
         ],
       },
@@ -793,7 +794,8 @@ export const visTypes = {
         label: t('Query'),
         expanded: true,
         controlSetRows: [
-          ['spatial', 'row_limit'],
+          ['spatial', null],
+          ['row_limit', 'filter_nulls'],
           ['adhoc_filters'],
         ],
       },

superset/viz.py

@@ -39,7 +39,12 @@
 
 from superset import app, cache, get_css_manifest_files, utils
 from superset.exceptions import NullValueException, SpatialException
-from superset.utils import DTTM_ALIAS, JS_MAX_INTEGER, merge_extra_filters
+from superset.utils import (
+    DTTM_ALIAS,
+    JS_MAX_INTEGER,
+    merge_extra_filters,
+    to_adhoc,
+)
 
 
 config = app.config
@@ -2070,17 +2075,19 @@ def get_metrics(self):
         return [self.metric] if self.metric else []
 
     def process_spatial_query_obj(self, key, group_by):
+        group_by.extend(self.get_spatial_columns(key))
+
+    def get_spatial_columns(self, key):
         spatial = self.form_data.get(key)
         if spatial is None:
             raise ValueError(_('Bad spatial key'))
 
         if spatial.get('type') == 'latlong':
-            group_by += [spatial.get('lonCol')]
-            group_by += [spatial.get('latCol')]
+            return [spatial.get('lonCol'), spatial.get('latCol')]
         elif spatial.get('type') == 'delimited':
-            group_by += [spatial.get('lonlatCol')]
+            return [spatial.get('lonlatCol')]
         elif spatial.get('type') == 'geohash':
-            group_by += [spatial.get('geohashCol')]
+            return [spatial.get('geohashCol')]
 
     @staticmethod
     def parse_coordinates(s):
@@ -2123,9 +2130,31 @@ def process_spatial_data_obj(self, key, df):
                                        please consider filtering those out'))
         return df
 
+    def add_null_filters(self):
+        spatial_columns = set()
+        for key in self.spatial_control_keys:
+            for column in self.get_spatial_columns(key):
+                spatial_columns.add(column)
+
+        if self.form_data.get('adhoc_filters') is None:
+            self.form_data['adhoc_filters'] = []
+
+        for column in sorted(spatial_columns):
+            filter_ = to_adhoc({
+                'col': column,
+                'op': 'IS NOT NULL',
+                'val': '',
+            })
+            self.form_data['adhoc_filters'].append(filter_)
+
     def query_obj(self):
-        d = super(BaseDeckGLViz, self).query_obj()
         fd = self.form_data
+
+        # add NULL filters
+        if fd.get('filter_nulls'):
+            self.add_null_filters()
+
+        d = super(BaseDeckGLViz, self).query_obj()
         gb = []
 
         for key in self.spatial_control_keys:

tests/viz_tests.py

@@ -6,6 +6,7 @@
 
 from datetime import datetime
 import unittest
+import uuid
 
 from mock import Mock, patch
 import pandas as pd
@@ -950,6 +951,67 @@ def test_parse_coordinates_raises(self):
         with self.assertRaises(SpatialException):
             test_viz_deckgl.parse_coordinates('fldkjsalkj,fdlaskjfjadlksj')
 
+    @patch('superset.utils.uuid.uuid4')
+    def test_filter_nulls(self, mock_uuid4):
+        mock_uuid4.return_value = uuid.UUID('12345678123456781234567812345678')
+        test_form_data = {
+            'latlong_key': {
+                'type': 'latlong',
+                'lonCol': 'lon',
+                'latCol': 'lat',
+            },
+            'delimited_key': {
+                'type': 'delimited',
+                'lonlatCol': 'lonlat',
+            },
+            'geohash_key': {
+                'type': 'geohash',
+                'geohashCol': 'geo',
+            },
+        }
+
+        datasource = {'type': 'table'}
+        expected_results = {
+            'latlong_key': [{
+                'clause': 'WHERE',
+                'expressionType': 'SIMPLE',
+                'filterOptionName': '12345678-1234-5678-1234-567812345678',
+                'comparator': '',
+                'operator': 'IS NOT NULL',
+                'subject': 'lat',
+            }, {
+                'clause': 'WHERE',
+                'expressionType': 'SIMPLE',
+                'filterOptionName': '12345678-1234-5678-1234-567812345678',
+                'comparator': '',
+                'operator': 'IS NOT NULL',
+                'subject': 'lon',
+            }],
+            'delimited_key': [{
+                'clause': 'WHERE',
+                'expressionType': 'SIMPLE',
+                'filterOptionName': '12345678-1234-5678-1234-567812345678',
+                'comparator': '',
+                'operator': 'IS NOT NULL',
+                'subject': 'lonlat',
+            }],
+            'geohash_key': [{
+                'clause': 'WHERE',
+                'expressionType': 'SIMPLE',
+                'filterOptionName': '12345678-1234-5678-1234-567812345678',
+                'comparator': '',
+                'operator': 'IS NOT NULL',
+                'subject': 'geo',
+            }],
+        }
+        for mock_key in ['latlong_key', 'delimited_key', 'geohash_key']:
+            test_viz_deckgl = viz.BaseDeckGLViz(
+                datasource, test_form_data.copy())
+            test_viz_deckgl.spatial_control_keys = [mock_key]
+            test_viz_deckgl.add_null_filters()
+            adhoc_filters = test_viz_deckgl.form_data['adhoc_filters']
+            assert expected_results.get(mock_key) == adhoc_filters
+
 
 class TimeSeriesVizTestCase(unittest.TestCase):