diff --git a/frontend/src/card/card.controllers.js b/frontend/src/card/card.controllers.js
index ccbd9c3c8112c28057298e0cf592f49a3025fb94..249bca54c3b95b9f0571599ee98d12c8ba97aca6 100644
--- a/frontend/src/card/card.controllers.js
+++ b/frontend/src/card/card.controllers.js
@@ -219,22 +219,16 @@ CardControllers.controller('CardDetail', [
renderAll();
},
setSortFn: function(fieldId) {
- // for now, just put this into the query and re-run
- var sortField = fieldId;
- if (fieldId === "agg") {
- sortField = ["aggregation", 0];
- }
-
// NOTE: we only allow this for structured type queries & we only allow sorting by a single column
if (card.dataset_query.type === "query") {
- var sortClause = [sortField, "ascending"];
+ var sortClause = [fieldId, "ascending"];
if (card.dataset_query.query.order_by &&
card.dataset_query.query.order_by.length > 0 &&
card.dataset_query.query.order_by[0].length > 0 &&
card.dataset_query.query.order_by[0][1] === "ascending" &&
- Query.isSameField(card.dataset_query.query.order_by[0][0], sortField)) {
+ Query.isSameField(card.dataset_query.query.order_by[0][0], fieldId)) {
// someone triggered another sort on the same column, so flip the sort direction
- sortClause = [sortField, "descending"];
+ sortClause = [fieldId, "descending"];
}
// set clause
diff --git a/frontend/src/lib/query.js b/frontend/src/lib/query.js
index 1ea69230285aa04d3b1b1e4e758cc267acfbd848..91c893c5f0b39751c5fac2388a1f6b1b183b9efb 100644
--- a/frontend/src/lib/query.js
+++ b/frontend/src/lib/query.js
@@ -59,34 +59,39 @@ var Query = {
query.order_by = query.order_by.map((s) => {
let field = s[0];
+ // remove incomplete sorts
+ if (!Query.isValidField(field) || s[1] == null) {
+ return null;
+ }
+
if (Query.isAggregateField(field)) {
// remove aggregation sort if we can't sort by this aggregation
- if (!Query.canSortByAggregateField(query)) {
- return null;
+ if (Query.canSortByAggregateField(query)) {
+ return s;
}
} else if (Query.hasValidBreakout(query)) {
- // remove sort if it references a non-broken-out field
- let matchingBreakout = query.breakout.filter(b => Query.isSameField(b, field));
- if (matchingBreakout.length > 0) {
- // query processor expect the order_by clause to match the breakout's granularity
- if (Array.isArray(matchingBreakout[0]) && matchingBreakout[0][0] === "datetime_field") {
- return [matchingBreakout[0], s[1]];
- } else {
- return s;
+ let exactMatches = query.breakout.filter(b => Query.isSameField(b, field, true));
+ if (exactMatches.length > 0) {
+ return s;
+ }
+ let targetMatches = query.breakout.filter(b => Query.isSameField(b, field, false));
+ if (targetMatches.length > 0) {
+ // query processor expect the order_by clause to match the breakout's datetime_field unit or fk-> target,
+ // so just replace it with the one that matches the target field
+ // NOTE: if we have more than one breakout for the same target field this could match the wrong one
+ if (targetMatches.length > 1) {
+ console.warn("Sort clause matches more than one breakout field", s[0], targetMatches);
}
- } else {
- return null;
+ return [targetMatches[0], s[1]];
}
- } else if (!Query.isBareRowsAggregation(query)) {
- // otherwise remove sort if it doesn't have a breakout but isn't a bare rows aggregation
- return null;
- }
- // remove incomplete sorts
- if (Query.isValidField(s[0]) && s[1] != null) {
+ } else if (Query.isBareRowsAggregation(query)) {
return s;
}
+
+ // otherwise remove sort if it doesn't have a breakout but isn't a bare rows aggregation
return null;
}).filter(s => s != null);
+
if (query.order_by.length === 0) {
delete query.order_by;
}
@@ -99,10 +104,6 @@ var Query = {
return query;
},
- isAggregateField: function(field) {
- return Array.isArray(field) && field[0] === "aggregation";
- },
-
canAddDimensions: function(query) {
var MAX_DIMENSIONS = 2;
return query && query.breakout && (query.breakout.length < MAX_DIMENSIONS);
@@ -347,49 +348,63 @@ var Query = {
}
},
+ isRegularField: function(field) {
+ return typeof field === "number";
+ },
+
+ isForeignKeyField: function(field) {
+ return Array.isArray(field) && field[0] === "fk->";
+ },
+
+ isDatetimeField: function(field) {
+ return Array.isArray(field) && field[0] === "datetime_field";
+ },
+
+ isAggregateField: function(field) {
+ return Array.isArray(field) && field[0] === "aggregation";
+ },
+
isValidField: function(field) {
return (
- typeof field === "number" ||
- (Array.isArray(field) && (
- (field[0] === 'fk->' && typeof field[1] === "number" && typeof field[2] === "number") ||
- (field[0] === 'datetime_field' && Query.isValidField(field[1]) && field[2] === "as" && typeof field[3] === "string") ||
- (field[0] === 'aggregation' && typeof field[1] === "number")
- ))
+ (Query.isRegularField(field)) ||
+ (Query.isForeignKeyField(field) && Query.isRegularField(field[1]) && Query.isRegularField(field[2])) ||
+ (Query.isDatetimeField(field) && Query.isValidField(field[1]) && field[2] === "as" && typeof field[3] === "string") ||
+ (Query.isAggregateField(field) && typeof field[1] === "number")
);
},
- isSameField: function(fieldA, fieldB, strict = false) {
- if (strict) {
+ isSameField: function(fieldA, fieldB, exact = false) {
+ if (exact) {
return _.isEqual(fieldA, fieldB);
} else {
return Query.getFieldTargetId(fieldA) === Query.getFieldTargetId(fieldB);
}
},
+ // gets the target field ID (recursively) from any type of field, including raw field ID, fk->, and datetime_field cast.
getFieldTargetId: function(field) {
- if (Array.isArray(field)) {
- if (field[0] === "fk->") {
- return field[2];
- }
- if (field[0] === "datetime_field") {
- return field[1];
- }
- } else {
+ if (Query.isRegularField(field)) {
return field;
+ } else if (Query.isForeignKeyField(field)) {
+ return Query.getFieldTargetId(field[2]);
+ } else if (Query.isDatetimeField(field)) {
+ return Query.getFieldTargetId(field[1]);
}
+ console.warn("Unknown field type: ", field);
},
- getFieldTarget: function(field, tableMetadata) {
- let table, fieldId, fk;
- if (Array.isArray(field) && field[0] === "fk->") {
- fk = tableMetadata.fields_lookup[field[1]];
- table = fk.target.table;
- fieldId = field[2];
- } else {
- table = tableMetadata;
- fieldId = field;
+ // gets the table and field definitions from from a raw, fk->, or datetime_field field
+ getFieldTarget: function(field, tableDef) {
+ if (Query.isRegularField(field)) {
+ return { table: tableDef, field: tableDef.fields_lookup[field] };
+ } else if (Query.isForeignKeyField(field)) {
+ let fkFieldDef = tableDef.fields_lookup[field[1]];
+ let targetTableDef = fkFieldDef.target.table;
+ return Query.getFieldTarget(field[2], targetTableDef);
+ } else if (Query.isDatetimeField(field)) {
+ return Query.getFieldTarget(field[1], tableDef);
}
- return { table, field: table.fields_lookup[fieldId] };
+ console.warn("Unknown field type: ", field);
},
getFieldOptions: function(fields, includeJoins = false, filterFn = (fields) => fields, usedFields = {}) {
@@ -403,7 +418,7 @@ var Query = {
results.count += results.fields.length;
if (includeJoins) {
results.fks = fields.filter((f) => f.special_type === "fk" && f.target).map((joinField) => {
- var targetFields = filterFn(joinField.target.table.fields).filter((f) => (!Array.isArray(f.id) || f.id[0] !== "aggregation") && !usedFields[f.id]);
+ var targetFields = filterFn(joinField.target.table.fields).filter(f => (!Array.isArray(f.id) || f.id[0] !== "aggregation") && !usedFields[f.id]);
results.count += targetFields.length;
return {
field: joinField,
@@ -420,15 +435,15 @@ var Query = {
}
function getFieldName(id, table) {
- if (Array.isArray(id)) {
- if (id[0] === "fk->") {
- var field = table.fields_lookup[id[1]];
- if (field) {
- return field.display_name.replace(/\s+id$/i, "") + " → " + getFieldName(id[2], field.target.table);
- }
- } else if (id[0] === "datetime_field") {
- return getFieldName(id[1], table) + " (by " + id[3] + ")";
+ if (Query.isForeignKeyField(id)) {
+ var field = table.fields_lookup[id[1]];
+ if (field) {
+ return field.display_name.replace(/\s+id$/i, "") + " → " + getFieldName(id[2], field.target.table);
}
+ } else if (Query.isDatetimeField(id)) {
+ return getFieldName(id[1], table) + " (by " + id[3] + ")";
+ } else if (Query.isAggregateField(id)) {
+ return "aggregation";
} else if (table.fields_lookup[id]) {
return table.fields_lookup[id].display_name
}
diff --git a/frontend/src/query_builder/QueryVisualizationTable.jsx b/frontend/src/query_builder/QueryVisualizationTable.jsx
index 64879b19e834d20bb25ebbe0b02d17794146fdd4..2024f89ae0c0af565fbbde4229363025dc9d927a 100644
--- a/frontend/src/query_builder/QueryVisualizationTable.jsx
+++ b/frontend/src/query_builder/QueryVisualizationTable.jsx
@@ -132,8 +132,15 @@ export default class QueryVisualizationTable extends Component {
return (this.props.setSortFn !== undefined);
}
- setSort(fieldId) {
- this.props.setSortFn(fieldId);
+ setSort(column) {
+ if (column.id == null) {
+ // ICK. this is hacky for dealing with aggregations. need something better
+ this.props.setSortFn(["aggregation", 0]);
+ } else if (column.unit != null) {
+ this.props.setSortFn(["datetime_field", column.id, "as", column.unit]);
+ } else {
+ this.props.setSortFn(column.id);
+ }
MetabaseAnalytics.trackEvent('QueryBuilder', 'Set Sort', 'table column');
}
@@ -243,11 +250,8 @@ export default class QueryVisualizationTable extends Component {
}
if (this.isSortable()) {
- // ICK. this is hacky for dealing with aggregations. need something better
- var fieldId = (column.id) ? column.id : "agg";
-
return (
- <div key={columnIndex} className={headerClasses} onClick={this.setSort.bind(this, fieldId)}>
+ <div key={columnIndex} className={headerClasses} onClick={this.setSort.bind(this, column)}>
<span>
{colVal}
</span>
diff --git a/frontend/test/unit/lib/query.spec.js b/frontend/test/unit/lib/query.spec.js
index e08332267b98fe4d48323486310502c2c5f84f3e..e6a78fb928f8fde1d3e4cba6c75ae14d6d820454 100644
--- a/frontend/test/unit/lib/query.spec.js
+++ b/frontend/test/unit/lib/query.spec.js
@@ -84,6 +84,62 @@ describe('Query', () => {
Query.cleanQuery(query);
expect(query.order_by).toBe(undefined);
});
+
+ it('should not remove sort clauses with foreign keys on fields appearing in breakout', () => {
+ let query = {
+ source_table: 0,
+ aggregation: ["count"],
+ breakout: [["fk->", 1, 2]],
+ filter: [],
+ order_by: [
+ [["fk->", 1, 2], "ascending"]
+ ]
+ };
+ Query.cleanQuery(query);
+ expect(JSON.stringify(query.order_by)).toBe(JSON.stringify([[["fk->", 1, 2], "ascending"]]));
+ });
+
+ it('should not remove sort clauses with datetime_fields on fields appearing in breakout', () => {
+ let query = {
+ source_table: 0,
+ aggregation: ["count"],
+ breakout: [["datetime_field", 1, "as", "week"]],
+ filter: [],
+ order_by: [
+ [["datetime_field", 1, "as", "week"], "ascending"]
+ ]
+ };
+ Query.cleanQuery(query);
+ expect(JSON.stringify(query.order_by)).toBe(JSON.stringify([[["datetime_field", 1, "as", "week"], "ascending"]]));
+ });
+
+ it('should replace order_by clauses with the exact matching datetime_fields version in the breakout', () => {
+ let query = {
+ source_table: 0,
+ aggregation: ["count"],
+ breakout: [["datetime_field", 1, "as", "week"]],
+ filter: [],
+ order_by: [
+ [1, "ascending"]
+ ]
+ };
+ Query.cleanQuery(query);
+ expect(JSON.stringify(query.order_by)).toBe(JSON.stringify([[["datetime_field", 1, "as", "week"], "ascending"]]));
+ });
+
+ it('should replace order_by clauses with the exact matching fk-> version in the breakout', () => {
+ let query = {
+ source_table: 0,
+ aggregation: ["count"],
+ breakout: [["fk->", 1, 2]],
+ filter: [],
+ order_by: [
+ [2, "ascending"]
+ ]
+ };
+ Query.cleanQuery(query);
+ expect(JSON.stringify(query.order_by)).toBe(JSON.stringify([[["fk->", 1, 2], "ascending"]]));
+ });
});
describe('removeDimension', () => {