From 533a957c4d588e6184b80ff69a933217e2f0aebf Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Fri, 9 Nov 2018 11:36:56 +0100 Subject: [PATCH 2/2] engine: make Dimension.order_by a list (fixes #28175) --- bijoe/engine.py | 143 ++++++++++++++++------------- bijoe/schemas.py | 2 +- tests/fixtures/schema1/schema.json | 8 ++ tests/test_schema1.py | 4 +- 4 files changed, 92 insertions(+), 65 deletions(-) diff --git a/bijoe/engine.py b/bijoe/engine.py index 80f21d2..96f390e 100644 --- a/bijoe/engine.py +++ b/bijoe/engine.py @@ -17,6 +17,7 @@ import contextlib import logging import itertools +import contextlib import collections import psycopg2 @@ -60,20 +61,32 @@ class EngineDimension(object): @property def members(self): assert self.type != 'date' + value = self.value + value_label = self.value_label or value + order_by = self.order_by + with self.engine.get_cursor() as cursor: sql = self.members_query if not sql: - if self.dimension.join: - join = self.engine_cube.get_join(self.dimension.join[-1]) - sql = ('SELECT %s AS value, %s::text AS label FROM %s AS "%s" ' - 'GROUP BY %s, %s ORDER BY %s' % ( - self.value, self.value_label or self.value, join.table, join.name, - self.value, self.value_label or self.value, self.order_by or self.value)) + table_expression = self.engine_cube.fact_table + if self.join: + table_expression = self.engine_cube.build_table_expression( + self.join, self.engine_cube.fact_table) + sql = 'SELECT %s AS value, %s::text AS label ' % (value, value_label) + sql += 'FROM %s ' % table_expression + if order_by: + if not isinstance(order_by, list): + order_by = [order_by] else: - sql = ('SELECT %s AS value, %s::text AS label FROM {fact_table} ' - 'GROUP BY %s, %s ORDER BY %s' % ( - self.value, self.value_label or self.value, self.value, - self.value_label or self.value, self.order_by or self.value)) + order_by = [value] + group_by = [value] + if value_label not in group_by: + group_by.append(value_label) + for order_value in order_by: + if order_value not in group_by: + group_by.append(order_value) + sql += 'GROUP BY %s ' % ', '.join(group_by) + sql += 'ORDER BY (%s) ' % ', '.join(order_by) sql = sql.format(fact_table=self.engine_cube.fact_table) self.engine.log.debug('SQL: %s', sql) cursor.execute(sql) @@ -196,48 +209,6 @@ class ProxyListDescriptor(object): return ProxyList(obj.engine, obj, self.attribute, self.cls, chain=self.chain) -def build_table_expression(join_tree, table_name, alias=None, top=True, other_conditions=None): - '''Recursively build the table expression from the join tree, - starting from the fact table''' - - sql = table_name - if alias: - sql += ' AS "%s"' % alias - add_paren = False - for kind in ['left', 'inner', 'right', 'full']: - joins = join_tree.get(table_name, {}).get(kind) - if not joins: - continue - add_paren = True - join_kinds = { - 'inner': 'INNER JOIN', - 'left': 'LEFT OUTER JOIN', - 'right': 'RIGHT OUTER JOIN', - 'full': 'FULL OUTER JOIN', - } - sql += ' %s ' % join_kinds[kind] - sub_joins = [] - conditions = [] - if other_conditions: - conditions = other_conditions - other_conditions = None - for join_name, join in joins.iteritems(): - sub_joins.append( - build_table_expression(join_tree, join.table, join.name, top=False)) - conditions.append('"%s".%s = "%s"."%s"' % ( - alias or table_name, - join.master.split('.')[-1], - join.name, join.detail)) - sub_join = ' CROSS JOIN '.join(sub_joins) - if len(sub_joins) > 1: - sub_join = '(%s)' % sub_join - sql += sub_join - sql += ' ON %s' % ' AND '.join(conditions) - if not top and add_paren: - sql = '(%s)' % sql - return sql - - class EngineCube(object): dimensions = ProxyListDescriptor('all_dimensions', EngineDimension, chain=JSONDimensions) measures = ProxyListDescriptor('measures', EngineMeasure) @@ -293,7 +264,11 @@ class EngineCube(object): projections.append('%s AS %s' % (dimension.value_label or dimension.value, dimension.name)) group_by.append(dimension.group_by or dimension.value) - order_by.append(dimension.order_by or dimension.value) + order_by.extend(dimension.order_by or [dimension.value]) + + for order_value in order_by: + if order_value not in group_by: + group_by.append(order_value) for measure_name in measures: measure = self.get_measure(measure_name) @@ -302,15 +277,8 @@ class EngineCube(object): sql = 'SELECT ' + ', '.join(projections) table_expression = ' %s' % self.cube.fact_table if joins: - join_tree = {} - # Build join tree - for join_name in joins: - join = self.get_join(join_name) - master_table = join.master_table or self.fact_table - join_tree.setdefault(master_table, {}).setdefault(join.kind, {})[join.name] = join - table_expression = build_table_expression(join_tree, - self.fact_table, - other_conditions=join_conditions) + table_expression = self.build_table_expression( + joins, self.fact_table, other_conditions=join_conditions) sql += ' FROM %s' % table_expression where_conditions = 'true' if where: @@ -349,6 +317,57 @@ class EngineCube(object): 'value': value, } for cell, value in zip(cells, row)] + def build_table_expression(self, joins, table_name, other_conditions=None): + '''Recursively build the table expression from the join tree, + starting from the fact table''' + + join_tree = {} + # Build join tree + for join_name in joins: + join = self.get_join(join_name) + master_table = join.master_table or self.fact_table + join_tree.setdefault(master_table, {}).setdefault(join.kind, {})[join.name] = join + + def build_table_expression_helper(join_tree, table_name, alias=None, top=True, other_conditions=None): + sql = table_name + if alias: + sql += ' AS "%s"' % alias + add_paren = False + for kind in ['left', 'inner', 'right', 'full']: + joins = join_tree.get(alias or table_name, {}).get(kind) + if not joins: + continue + add_paren = True + join_kinds = { + 'inner': 'INNER JOIN', + 'left': 'LEFT OUTER JOIN', + 'right': 'RIGHT OUTER JOIN', + 'full': 'FULL OUTER JOIN', + } + sql += ' %s ' % join_kinds[kind] + sub_joins = [] + conditions = [] + if other_conditions: + conditions = other_conditions + other_conditions = None + for join_name, join in joins.iteritems(): + sub_joins.append( + build_table_expression_helper(join_tree, join.table, alias=join.name, top=False)) + conditions.append('"%s".%s = "%s"."%s"' % ( + alias or table_name, + join.master.split('.')[-1], + join.name, join.detail)) + sub_join = ' CROSS JOIN '.join(sub_joins) + if len(sub_joins) > 1: + sub_join = '(%s)' % sub_join + sql += sub_join + sql += ' ON %s' % ' AND '.join(conditions) + if not top and add_paren: + sql = '(%s)' % sql + return sql + return build_table_expression_helper( + join_tree, table_name, other_conditions=other_conditions) + class Engine(object): def __init__(self, warehouse): diff --git a/bijoe/schemas.py b/bijoe/schemas.py index 6dd5bb5..c16993c 100644 --- a/bijoe/schemas.py +++ b/bijoe/schemas.py @@ -143,7 +143,7 @@ class Dimension(Base): 'join': [str], 'value': str, 'value_label': str, - 'order_by': str, + 'order_by': [str], 'group_by': str, 'filter': bool, 'members_query': str, diff --git a/tests/fixtures/schema1/schema.json b/tests/fixtures/schema1/schema.json index 9b3156d..3c8b3ae 100644 --- a/tests/fixtures/schema1/schema.json +++ b/tests/fixtures/schema1/schema.json @@ -86,6 +86,7 @@ "type": "integer", "join": ["innercategory", "innersubcategory"], "value": "innersubcategory.id", + "order_by": ["innercategory.ord", "innersubcategory.ord", "innersubcategory.label"], "value_label": "innersubcategory.label" }, { @@ -94,6 +95,7 @@ "type": "integer", "join": ["leftcategory", "leftsubcategory"], "value": "leftsubcategory.id", + "order_by": ["leftcategory.ord", "leftsubcategory.ord", "leftsubcategory.label"], "value_label": "leftsubcategory.label" }, { @@ -102,6 +104,7 @@ "type": "integer", "join": ["rightcategory", "rightsubcategory"], "value": "rightsubcategory.id", + "order_by": ["rightcategory.ord", "rightsubcategory.ord", "rightsubcategory.label"], "value_label": "rightsubcategory.label" }, { @@ -110,6 +113,7 @@ "type": "integer", "join": ["outercategory", "outersubcategory"], "value": "outersubcategory.id", + "order_by": ["outercategory.ord", "outersubcategory.ord", "outersubcategory.label"], "value_label": "outersubcategory.label" }, { @@ -118,6 +122,7 @@ "type": "integer", "join": ["innersubcategory", "innercategory"], "value": "innercategory.id", + "order_by": "innercategory.ord", "value_label": "innercategory.label" }, { @@ -126,6 +131,7 @@ "type": "integer", "join": ["leftsubcategory", "leftcategory"], "value": "leftcategory.id", + "order_by": "leftcategory.ord", "value_label": "leftcategory.label" }, { @@ -134,6 +140,7 @@ "type": "integer", "join": ["rightsubcategory", "rightcategory"], "value": "rightcategory.id", + "order_by": "rightcategory.ord", "value_label": "rightcategory.label" }, { @@ -142,6 +149,7 @@ "type": "integer", "join": ["outersubcategory", "outercategory"], "value": "outercategory.id", + "order_by": "outercategory.ord", "value_label": "outercategory.label" } ], diff --git a/tests/test_schema1.py b/tests/test_schema1.py index b46495f..8bd5860 100644 --- a/tests/test_schema1.py +++ b/tests/test_schema1.py @@ -24,8 +24,8 @@ def test_simple(schema1, app, admin): response = form.submit('visualize') assert 'big-msg-info' not in response assert get_table(response) == [ - [u'Inner SubCategory', u'subé1', u'subé3'], - ['number of rows', '15', '1'], + [u'Inner SubCategory', u'subé3', u'subé1'], + ['number of rows', '1', '15'], ] form = response.form form.set('representation', 'table') -- 2.18.0