Unda the MSSQL specific changes regarding GROUP BY for rollups as aft…

…er the blending query optimization they are no longer required

fireant/database/base.py

@@ -22,7 +22,6 @@ class Database(object):
     query_cls = Query
 
     slow_query_log_min_seconds = 15
-    can_group_static_value = True
 
     def __init__(
         self,

fireant/database/mssql.py

@@ -17,7 +17,6 @@ class MSSQLDatabase(Database):
 
     # The pypika query class to use for constructing queries
     query_cls = MSSQLQuery
-    can_group_static_value = False
 
     def __init__(
         self,

fireant/dataset/fields.py

@@ -145,6 +145,14 @@ def __init__(
     def is_aggregate(self):
         return self.definition.is_aggregate
 
+    @property
+    def groupable(self):
+        """
+        Whether this field should be allowed to be specified
+        in a query group by statement
+        """
+        return not self.is_aggregate
+
     @property
     def is_wrapped(self) -> bool:
         """

fireant/dataset/modifiers.py

@@ -102,6 +102,13 @@ class Rollup(DimensionModifier):
     """
     A field modifier that will make totals be calculated for the wrapped dimension.
     """
+    @property
+    def groupable(self):
+        """
+        Do not use rollups in group bys - this is superfluous and some database
+        engines do not support grouping on static columns e.g. MSSQL
+        """
+        return False
 
     @property
     def definition(self):

fireant/queries/sql_transformer.py

@@ -168,13 +168,7 @@ def make_slicer_query(
         dimension_term = make_term_for_field(dimension, database.trunc_date)
         query = query.select(dimension_term)
 
-        # Some database platforms like MSSQL do not support grouping by static value columns.
-        # Fireant uses static value columns for totals placeholders.
-        # TODO this can be reverted once an issue with data blending attaching unnecessary subqueries
-        # is removed as in some cases the left join can cause duplicate rows to appear when not grouping a rollup column
-        ungroupable_rollup = isinstance(dimension, Rollup) and not database.can_group_static_value
-
-        if not dimension.is_aggregate and not ungroupable_rollup:
+        if dimension.groupable:
             query = query.groupby(dimension_term)
 
     # Add filters

fireant/tests/queries/test_build_data_blending.py

@@ -805,15 +805,15 @@ def test_apply_totals_to_blended_query(self):
                 "'_FIREANT_ROLLUP_VALUE_' \"$candidate-id\","
                 'SUM("is_winner") "$wins" '
                 'FROM "politics"."politician" '
-                'GROUP BY "$timestamp","$candidate-id"'
+                'GROUP BY "$timestamp"'
                 ') "sq0" '
                 "LEFT JOIN ("
                 "SELECT "
                 'TRUNC("timestamp",\'DD\') "$timestamp",'
                 "'_FIREANT_ROLLUP_VALUE_' \"$candidate-id\","
                 'SUM("candidate_spend") "$candidate-spend" '
                 'FROM "politics"."politician_spend" '
-                'GROUP BY "$timestamp","$candidate-id"'
+                'GROUP BY "$timestamp"'
                 ') "sq1" '
                 "ON "
                 '"sq0"."$timestamp"="sq1"."$timestamp" '

fireant/tests/queries/test_build_dimensions.py

@@ -285,7 +285,6 @@ def test_build_query_with_single_rollup_dimension(self):
                 "'_FIREANT_ROLLUP_VALUE_' \"$political_party\","
                 'SUM("votes") "$votes" '
                 'FROM "politics"."politician" '
-                'GROUP BY "$political_party" '
                 'ORDER BY "$political_party" '
                 'LIMIT 200000',
                 str(queries[1]),
@@ -329,7 +328,7 @@ def test_build_query_with_totals_on_dimension_and_subsequent_dimensions(self):
                 "'_FIREANT_ROLLUP_VALUE_' \"$political_party\","
                 'SUM("votes") "$votes" '
                 'FROM "politics"."politician" '
-                'GROUP BY "$timestamp","$candidate-id","$political_party" '
+                'GROUP BY "$timestamp" '
                 'ORDER BY "$timestamp","$candidate-id","$political_party" '
                 'LIMIT 200000',
                 str(queries[1]),
@@ -373,7 +372,7 @@ def test_build_query_with_rollup_multiple_dimensions(self):
                 "'_FIREANT_ROLLUP_VALUE_' \"$political_party\","
                 'SUM("votes") "$votes" '
                 'FROM "politics"."politician" '
-                'GROUP BY "$timestamp","$candidate-id","$political_party" '
+                'GROUP BY "$timestamp","$candidate-id" '
                 'ORDER BY "$timestamp","$candidate-id","$political_party" '
                 'LIMIT 200000',
                 str(queries[1]),
@@ -390,7 +389,7 @@ def test_build_query_with_rollup_multiple_dimensions(self):
                 "'_FIREANT_ROLLUP_VALUE_' \"$political_party\","
                 'SUM("votes") "$votes" '
                 'FROM "politics"."politician" '
-                'GROUP BY "$timestamp","$candidate-id","$political_party" '
+                'GROUP BY "$timestamp" '
                 'ORDER BY "$timestamp","$candidate-id","$political_party" '
                 'LIMIT 200000',
                 str(queries[2]),
@@ -445,7 +444,7 @@ def test_build_query_with_rollup_dimension_and_a_reference(self):
                 "'_FIREANT_ROLLUP_VALUE_' \"$political_party\","
                 'SUM("votes") "$votes" '
                 'FROM "politics"."politician" '
-                'GROUP BY "$timestamp","$political_party" '
+                'GROUP BY "$timestamp" '
                 'ORDER BY "$timestamp","$political_party" '
                 'LIMIT 200000',
                 str(queries[2]),
@@ -460,7 +459,7 @@ def test_build_query_with_rollup_dimension_and_a_reference(self):
                 "'_FIREANT_ROLLUP_VALUE_' \"$political_party\","
                 'SUM("votes") "$votes_dod" '
                 'FROM "politics"."politician" '
-                'GROUP BY "$timestamp","$political_party" '
+                'GROUP BY "$timestamp" '
                 'ORDER BY "$timestamp","$political_party" '
                 'LIMIT 200000',
                 str(queries[3]),
@@ -524,7 +523,7 @@ def test_build_query_with_rollup_cat_dimension_with_references_and_date_filters(
                 'SUM("votes") "$votes" '
                 'FROM "politics"."politician" '
                 "WHERE \"timestamp\" BETWEEN '2018-01-01' AND '2019-01-01' "
-                'GROUP BY "$timestamp","$political_party" '
+                'GROUP BY "$timestamp" '
                 'ORDER BY "$timestamp","$political_party" '
                 'LIMIT 200000',
                 str(queries[2]),
@@ -541,7 +540,7 @@ def test_build_query_with_rollup_cat_dimension_with_references_and_date_filters(
                 'FROM "politics"."politician" '
                 "WHERE \"timestamp\" BETWEEN TIMESTAMPADD('day',-1,'2018-01-01') "
                 "AND TIMESTAMPADD('day',-1,'2019-01-01') "
-                'GROUP BY "$timestamp","$political_party" '
+                'GROUP BY "$timestamp" '
                 'ORDER BY "$timestamp","$political_party" '
                 'LIMIT 200000',
                 str(queries[3]),
@@ -590,7 +589,7 @@ def test_build_query_with_rollup_dimension_and_total_filter_not_applied(self):
                 "'_FIREANT_ROLLUP_VALUE_' \"$timestamp\","
                 'SUM("votes") "$votes" '
                 'FROM "politics"."politician" '
-                'GROUP BY "$political_party","$timestamp" '
+                'GROUP BY "$political_party" '
                 'ORDER BY "$political_party","$timestamp" '
                 'LIMIT 200000',
                 str(queries[1]),
@@ -646,7 +645,7 @@ def test_build_query_with_rollup_dimension_and_two_filters_only_one_applied_to_r
                 'SUM("votes") "$votes" '
                 'FROM "politics"."politician" '
                 "WHERE \"timestamp\" BETWEEN '2018-03-01' AND '2019-09-01' "
-                'GROUP BY "$political_party","$timestamp" '
+                'GROUP BY "$political_party" '
                 'ORDER BY "$political_party","$timestamp" '
                 'LIMIT 200000',
                 str(queries[1]),
@@ -697,7 +696,7 @@ def test_build_query_with_rollup_dimensions_and_filter_applied_on_correct_rollup
                 "'_FIREANT_ROLLUP_VALUE_' \"$timestamp\","
                 'SUM("votes") "$votes" '
                 'FROM "politics"."politician" '
-                'GROUP BY "$political_party","$timestamp" '
+                'GROUP BY "$political_party" '
                 'ORDER BY "$political_party","$timestamp" '
                 'LIMIT 200000',
                 str(queries[1]),
@@ -712,7 +711,6 @@ def test_build_query_with_rollup_dimensions_and_filter_applied_on_correct_rollup
                 "'_FIREANT_ROLLUP_VALUE_' \"$timestamp\","
                 'SUM("votes") "$votes" '
                 'FROM "politics"."politician" '
-                'GROUP BY "$political_party","$timestamp" '
                 'ORDER BY "$political_party","$timestamp" '
                 'LIMIT 200000',
                 str(queries[2]),

fireant/tests/queries/test_build_references.py

@@ -1110,7 +1110,6 @@ def test_reference_with_rollup_dimension_and_date_range_filter(self):
                 'SUM("votes") "$votes" '
                 'FROM "politics"."politician" '
                 "WHERE \"timestamp\" BETWEEN '2018-01-01' AND '2018-01-31' "
-                'GROUP BY "$timestamp" '
                 'ORDER BY "$timestamp" '
                 'LIMIT 200000',
                 str(base_rollup),
@@ -1126,7 +1125,6 @@ def test_reference_with_rollup_dimension_and_date_range_filter(self):
                 'FROM "politics"."politician" '
                 "WHERE \"timestamp\" BETWEEN TIMESTAMPADD('week',-1,'2018-01-01') "
                 "AND TIMESTAMPADD('week',-1,'2018-01-31') "
-                'GROUP BY "$timestamp" '
                 'ORDER BY "$timestamp" '
                 'LIMIT 200000',
                 str(reference_rollup),

fireant/tests/queries/test_build_sets.py

@@ -127,7 +127,7 @@ def test_dimension_with_dimension_modifier_is_replaced_by_default_when_result_se
                 '\'_FIREANT_ROLLUP_VALUE_\' "$boolean",'
                 'SUM("number") "$aggr_number" '
                 'FROM "test" '
-                'GROUP BY "$date","$boolean" '
+                'GROUP BY "$date" '
                 'ORDER BY "$date","$boolean" '
                 'LIMIT 200000',
                 str(queries[1]),

fireant/tests/queries/test_data_blending_integration.py

@@ -1054,8 +1054,7 @@ def test_blending_with_omit_from_rollup_filter_of_blended_field(self):
             "LEFT JOIN ("
             "SELECT "
             "'_FIREANT_ROLLUP_VALUE_' \"$timestamp\" "
-            'FROM "test1" '
-            'GROUP BY "$timestamp"'
+            'FROM "test1"'
             ') "sq1" ON "sq0"."$timestamp"="sq1"."$timestamp" '
             'ORDER BY "$timestamp" '
             'LIMIT 200000',
@@ -1313,15 +1312,13 @@ def test_share_on_blended_metric(self):
             "SELECT "
             "'_FIREANT_ROLLUP_VALUE_' \"$timestamp\","
             '"metric" "$metric0" '
-            'FROM "test0" '
-            'GROUP BY "$timestamp"'
+            'FROM "test0"'
             ') "sq0" '
             "LEFT JOIN ("
             "SELECT "
             "'_FIREANT_ROLLUP_VALUE_' \"$timestamp\","
             '"metric" "$metric1" '
-            'FROM "test1" '
-            'GROUP BY "$timestamp"'
+            'FROM "test1"'
             ') "sq1" ON "sq0"."$timestamp"="sq1"."$timestamp" '
             'ORDER BY "$timestamp" '
             'LIMIT 200000',