Repository: CartoDB/cartoframes

Issue:
null column values in styling not handled correctly
Dataset is earthquakes and styling for both color and size is ramp of magnitude (5 bins, equal interval). Null values in the column used for styling are being colored as though they are in the highest color bin using CARTOframes.

![image](https://user-images.githubusercontent.com/12551177/32517488-9956e03a-c3d4-11e7-9786-9e4b60c096e1.png)

Equivalent styling in CARTO Builder shows that the null values default to the lowest color in the ramp.

![image](https://user-images.githubusercontent.com/12551177/32517628-0152530e-c3d5-11e7-91ea-d70f1340be9f.png)

We should change this so the null values correspond to the lowest value of ramp, the same as in Builder.


Discussion:

Patch:
diff --git a/cartoframes/context.py b/cartoframes/context.py
index acac1b3e..f05c9f3b 100644
--- a/cartoframes/context.py
+++ b/cartoframes/context.py
@@ -1013,6 +1013,9 @@ class CartoContext(object):
                     query=layer.orig_query,
                     geoms=','.join(g['geom_type'] for g in resp['rows']),
                     common_geom=resp['rows'][0]['geom_type']))
+        elif len(resp['rows']) == 0:
+            raise ValueError('No geometry for layer. Check all layer tables '
+                             'and queries to ensure there are geometries.')
         return resp['rows'][0]['geom_type']
 
     def data_boundaries(self, df=None, table_name=None):
@@ -1303,8 +1306,8 @@ class CartoContext(object):
                 median_income = cc.data_discovery('transaction_events',
                                                   regex='.*median income.*',
                                                   time='2011 - 2015')
-                df = cc.data(median_income,
-                             'transaction_event')
+                df = cc.data('transaction_events',
+                             median_income)
 
             Pass in cherry-picked measures from the Data Observatory catalog.
             The rest of the metadata will be filled in, but it's important to
diff --git a/cartoframes/layer.py b/cartoframes/layer.py
index a10acf78..789344c5 100644
--- a/cartoframes/layer.py
+++ b/cartoframes/layer.py
@@ -7,7 +7,7 @@ basemap layers.
 import pandas as pd
 import webcolors
 
-from cartoframes.utils import cssify, join_url
+from cartoframes.utils import cssify, join_url, minify_sql
 from cartoframes.styling import BinMethod, mint, antique, get_scheme_cartocss
 
 # colors map data layers without color specified
@@ -388,7 +388,7 @@ class QueryLayer(AbstractLayer):
             duration = self.time['duration']
             if (self.color in self.style_cols and
                     self.style_cols[self.color] in ('string', 'boolean', )):
-                self.query = ' '.join([s.strip() for s in [
+                self.query = minify_sql([
                     'SELECT',
                     '    orig.*, __wrap.cf_value_{col}',
                     'FROM ({query}) AS orig, (',
@@ -404,7 +404,7 @@ class QueryLayer(AbstractLayer):
                     '    ) AS _wrap',
                     ') AS __wrap',
                     'WHERE __wrap.{col} = orig.{col}',
-                ]]).format(col=self.color, query=self.orig_query)
+                ]).format(col=self.color, query=self.orig_query)
                 agg_func = '\'CDB_Math_Mode(cf_value_{})\''.format(self.color)
                 self.scheme = {
                         'bins': ','.join(str(i) for i in range(1, 11)),
@@ -476,6 +476,11 @@ class QueryLayer(AbstractLayer):
                     'comp-op': 'source-over',
                 }
             })
+            if self.color in self.style_cols:
+                css += cssify({
+                    '#layer[{} = null]'.format(self.color): {
+                        'marker-fill': '#666'}
+                    })
             for t in range(1, self.time['trails'] + 1):
                 # Trails decay as 1/2^n, and grow 30% at each step
                 trail_temp = cssify({
@@ -487,33 +492,56 @@ class QueryLayer(AbstractLayer):
                 css += trail_temp
             return css
         else:
-            return cssify({
-                # Point CSS
-                "#layer['mapnik::geometry_type'=1]": {
-                    'marker-width': size_style,
-                    'marker-fill': color_style,
-                    'marker-fill-opacity': '1',
-                    'marker-allow-overlap': 'true',
-                    'marker-line-width': '0.5',
-                    'marker-line-color': line_color,
-                    'marker-line-opacity': '1',
-                },
-                # Line CSS
-                "#layer['mapnik::geometry_type'=2]": {
-                    'line-width': '1.5',
-                    'line-color': color_style,
-                },
-                # Polygon CSS
-                "#layer['mapnik::geometry_type'=3]": {
-                    'polygon-fill': color_style,
-                    'polygon-opacity': '0.9',
-                    'polygon-gamma': '0.5',
-                    'line-color': '#FFF',
-                    'line-width': '0.5',
-                    'line-opacity': '0.25',
-                    'line-comp-op': 'hard-light',
-                }
-            })
+            if self.geom_type == 'point':
+                css = cssify({
+                    # Point CSS
+                    "#layer": {
+                        'marker-width': size_style,
+                        'marker-fill': color_style,
+                        'marker-fill-opacity': '1',
+                        'marker-allow-overlap': 'true',
+                        'marker-line-width': '0.5',
+                        'marker-line-color': line_color,
+                        'marker-line-opacity': '1',
+                    }})
+                if self.color in self.style_cols:
+                    css += cssify({
+                        '#layer[{} = null]'.format(self.color): {
+                            'marker-fill': '#ccc'}
+                        })
+                return css
+            elif self.geom_type == 'line':
+                css = cssify({
+                    "#layer": {
+                        'line-width': '1.5',
+                        'line-color': color_style,
+                    }})
+                if self.color in self.style_cols:
+                    css += cssify({
+                        '#layer[{} = null]'.format(self.color): {
+                            'line-color': '#ccc'}
+                        })
+                return css
+            elif self.geom_type == 'polygon':
+                css = cssify({
+                    "#layer": {
+                        'polygon-fill': color_style,
+                        'polygon-opacity': '0.9',
+                        'polygon-gamma': '0.5',
+                        'line-color': '#FFF',
+                        'line-width': '0.5',
+                        'line-opacity': '0.25',
+                        'line-comp-op': 'hard-light',
+                    }})
+                if self.color in self.style_cols:
+                    css += cssify({
+                        '#layer[{} = null]'.format(self.color): {
+                            'polygon-fill': '#ccc'}
+                        })
+                return css
+            else:
+                raise ValueError('Unsupported geometry type: {}'.format(
+                    self.geom_type))
 
 
 class Layer(QueryLayer):
diff --git a/cartoframes/utils.py b/cartoframes/utils.py
index 820398bc..36365850 100644
--- a/cartoframes/utils.py
+++ b/cartoframes/utils.py
@@ -17,7 +17,7 @@ def cssify(css_dict):
             css += ' {field}: {field_value};'.format(field=field,
                                                      field_value=field_value)
         css += '} '
-    return css
+    return css.strip()
 
 
 def normalize_colnames(columns):


Test patch:
diff --git a/test/test_context.py b/test/test_context.py
index cdc1d0c6..87fb4383 100644
--- a/test/test_context.py
+++ b/test/test_context.py
@@ -566,6 +566,16 @@ class TestCartoContext(unittest.TestCase):
             cc.map(layers=[Layer(self.test_read_table, time='cartodb_id'),
                            Layer(self.test_read_table, time='cartodb_id')])
 
+        # no geometry
+        with self.assertRaises(ValueError):
+            cc.map(layers=QueryLayer('''
+                SELECT
+                    null::geometry as the_geom,
+                    null::geometry as the_geom_webmercator,
+                    row_number() OVER () as cartodb_id
+                FROM generate_series(1, 10) as m(i)
+                '''))
+
     @unittest.skipIf(WILL_SKIP, 'no cartocredentials, skipping')
     def test_cartocontext_map_time(self):
         """CartoContext.map time options"""
diff --git a/test/test_layer.py b/test/test_layer.py
index 8e1699e5..806afe2d 100644
--- a/test/test_layer.py
+++ b/test/test_layer.py
@@ -145,6 +145,7 @@ class TestQueryLayer(unittest.TestCase):
 
         for idx, color in enumerate(str_colors):
             qlayer = QueryLayer(self.query, color=color)
+            qlayer.geom_type = 'point'
             if color == 'cookie_monster':
                 qlayer.style_cols[color] = 'number'
                 qlayer._setup([BaseMap(), qlayer], 1)
@@ -159,6 +160,7 @@ class TestQueryLayer(unittest.TestCase):
             qlayer = QueryLayer(self.query, color='datetime_column')
             qlayer.style_cols['datetime_column'] = 'date'
             qlayer._setup([BaseMap(), qlayer], 1)
+
         # Exception testing
         # color column cannot be a geometry column
         with self.assertRaises(ValueError,
@@ -192,10 +194,12 @@ class TestQueryLayer(unittest.TestCase):
                              dict(name='Antique', bin_method='',
                                   bins=','.join(str(i) for i in range(1, 11))))
         # expect category maps query
+        with open('qlayerquery.txt', 'w') as f:
+            f.write(ql.query)
         self.assertRegexpMatches(ql.query,
-                                 '^SELECT orig\.\*, '
-                                 '__wrap.cf_value_colorcol.* '
-                                 'GROUP BY.*orig\.colorcol$')
+                                 '(?s)^SELECT\norig\.\*,\s__wrap\.'
+                                 'cf_value_colorcol\n.*GROUP\sBY.*orig\.'
+                                 'colorcol$')
         # cartocss should have cdb math mode
         self.assertRegexpMatches(ql.cartocss,
                                  '.*CDB_Math_Mode\(cf_value_colorcol\).*')
@@ -346,8 +350,31 @@ class TestQueryLayer(unittest.TestCase):
         """layer.QueryLayer._get_cartocss"""
         qlayer = QueryLayer(self.query, size=dict(column='cold_brew', min=10,
                                                   max=20))
+        qlayer.geom_type = 'point'
         self.assertRegexpMatches(
             qlayer._get_cartocss(BaseMap()),
             ('.*marker-width:\sramp\(\[cold_brew\],\srange\(10,20\),\s'
              'quantiles\(5\)\).*')
         )
+
+        # test line cartocss
+        qlayer = QueryLayer(self.query)
+        qlayer.geom_type = 'line'
+        self.assertRegexpMatches(qlayer._get_cartocss(BaseMap()),
+                                 '^\#layer.*line\-width.*$')
+        # test point, line, polygon
+        for g in ('point', 'line', 'polygon', ):
+            styles = {'point': 'marker\-fill',
+                      'line': 'line\-color',
+                      'polygon': 'polygon\-fill'}
+            qlayer = QueryLayer(self.query, color='colname')
+            qlayer.geom_type = g
+            self.assertRegexpMatches(qlayer._get_cartocss(BaseMap()),
+                                     '^\#layer.*{}.*\}}$'.format(styles[g]))
+
+        # geometry type should be defined
+        with self.assertRaises(ValueError,
+                               msg='invalid geometry type'):
+            ql = QueryLayer(self.query, color='red')
+            ql.geom_type = 'notvalid'
+            ql._get_cartocss(BaseMap())
diff --git a/test/test_utils.py b/test/test_utils.py
index 4be5fb25..af4db384 100644
--- a/test/test_utils.py
+++ b/test/test_utils.py
@@ -85,7 +85,7 @@ class TestUtils(unittest.TestCase):
                           "marker-width: 6; marker-fill: yellow; "
                           "marker-fill-opacity: 1; marker-allow-overlap: "
                           "true; marker-line-width: 0.5; marker-line-color: "
-                          "black; marker-line-opacity: 1;} "),
+                          "black; marker-line-opacity: 1;}"),
                          msg="point style")
 
         # polygon style
@@ -96,7 +96,7 @@ class TestUtils(unittest.TestCase):
                           "#cc607d, #9e3963, #672044), quantiles); "
                           "polygon-opacity: 0.9; polygon-gamma: 0.5; "
                           "line-color: #FFF; line-width: 0.5; line-opacity: "
-                          "0.25; line-comp-op: hard-light;} "),
+                          "0.25; line-comp-op: hard-light;}"),
                          msg="polygon style")
 
         # complex style
@@ -113,7 +113,7 @@ class TestUtils(unittest.TestCase):
                           "polygon-fill: blue; polygon-opacity: 0.9; "
                           "polygon-gamma: 0.5; line-color: #FFF; line-width: "
                           "0.5; line-opacity: 0.25; "
-                          "line-comp-op: hard-light;} "),
+                          "line-comp-op: hard-light;}"),
                          msg="multi-layer styling")
 
     def test_norm_colname(self):