code review

hammerlab · Nov 13, 2014 · ab89c6e · ab89c6e
1 parent 6338820
commit ab89c6e
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 15 deletions.
diff --git a/__tests__/js/query-test.js b/__tests__/js/query-test.js
@@ -5,7 +5,7 @@ var _ = require('underscore'),
     QueryLanguage = require('../../cycledash/static/js/QueryLanguage.js');
 
 
-describe('Query Langauge', function() {
+describe('Query Language', function() {
   var columns = ['A', 'B', 'INFO.DP'];
 
   function expectParse(query, expectedResult) {
@@ -23,7 +23,7 @@ describe('Query Langauge', function() {
   });
 
   it('should parse ranges', function() {
-    expectParse('20', {range: {contig: '20'}});
+    expectParse('20:', {range: {contig: '20'}});
     expectParse('20:1234-', {range: {contig: '20', start: '1234'}});
     expectParse('20:-4,567', {range: {contig: '20', end: '4567'}});
     expectParse('X:345-4,567', {range: {contig: 'X', start: '345', end: '4567'}});
@@ -55,6 +55,29 @@ describe('Query Langauge', function() {
     });
   });
 
+  it('should parse compound order bys', function() {
+    expectParse('ORDER BY A, B', {
+      sortBy:[{order:'asc', columnName: ['A']},
+              {order:'asc', columnName: ['B']}]
+    });
+
+    expectParse('ORDER BY A DESC, B', {
+      sortBy:[{order:'desc', columnName: ['A']},
+              {order:'asc', columnName: ['B']}]
+    });
+
+    expectParse('ORDER BY A ASC, B ASC', {
+      sortBy:[{order:'asc', columnName: ['A']},
+              {order:'asc', columnName: ['B']}]
+    });
+
+    expectParse('ORDER BY A ASC, B, INFO.DP DESC', {
+      sortBy:[{order:'asc', columnName: ['A']},
+              {order:'asc', columnName: ['B']},
+              {order:'desc', columnName: ['INFO', 'DP']}]
+    });
+  });
+
   it('should parse everything together', function() {
     expectParse('A <= 10 AND X:345-4,567 AND B = ABC ORDER BY INFO.DP ASC',
                 {

diff --git a/cycledash/static/js/QueryLanguage.js b/cycledash/static/js/QueryLanguage.js
@@ -29,7 +29,7 @@ function parse(query, columnNames) {
   // Massage this into the JSON format that the backend expects.
 
   // Group by type & remove the type tags.
-  var {filter,range,sort} = _.groupBy(parsedQuery, (info) => {
+  var {filter, range, sort} = _.groupBy(parsedQuery, (info) => {
     var type = info.type;
     delete info.type;
     return type;
@@ -38,6 +38,13 @@ function parse(query, columnNames) {
   if (range && range.length > 1) {
     return {error: 'You may only specify one range (got ' + range.length + ')'};
   }
+  if (sort) {
+    if (sort.length > 1) {
+      throw "Bug in the grammar: should only have one sort.";
+    } else {
+      sort = sort[0];
+    }
+  }
 
   // Flatten range structs
   if (range) {
@@ -51,7 +58,7 @@ function parse(query, columnNames) {
 
   // Validate fields, converting to column names as we go.
   var errors = [];
-  _.union(filter,sort).forEach((item) => {
+  _.union(filter, sort && sort.fields).forEach((item) => {
     var field = item.field;
     if (!_.contains(columnNames, field)) {
       errors.push({error: 'Unknown field ' + field});
@@ -75,10 +82,11 @@ function parse(query, columnNames) {
 
   // Normalize "sort" fields.
   if (sort) {
-    sort.forEach((o) => {
+    sort.fields.forEach((o) => {
       if (!o.order) o.order = 'asc';
       o.order = o.order.toLowerCase();
     });
+    sort = sort.fields;
   }
 
   return dropFalsyValues({filters: filter, sortBy: sort, range: range});

diff --git a/grammars/querylanguage.pegjs b/grammars/querylanguage.pegjs
@@ -4,13 +4,13 @@
  * Sample queries that it can parse:
  *   A < 10
  *   B = ABC
- *   20
+ *   20:
  *   20:1234-
  *   20:-4,567
  *   X:345-4,567
  *   ORDER BY A
  *   ORDER BY B DESC
- *   ORDER BY INFO.DP ASC
+ *   ORDER BY A, INFO.DP ASC
  *   A < 10 AND B >= ABC
  *   20:1234- AND A < 10
  *   A <= 10 and X:345-4,567 and B = ABC ORDER BY INFO.DP ASC
@@ -36,18 +36,18 @@ filter_or_range
   = filter
   / range
 
-filter
+filter "filter"
   = k:field ws op:op ws v:value { return {type: 'filter', field: k, op: op, value: v} }
 
-field
+field "field"
   = chars:[0-9a-z.]i+ { return chars.join(''); }
 
-value
+value "value"
   = chars:[0-9a-z.]i+ { return chars.join(''); }
   / doublequote chars:[^"]* doublequote { return chars.join(''); }
   / singlequote chars:[^']* singlequote { return chars.join(''); }
 
-op
+op "op"
   = "<="
   / "<"
   / ">="
@@ -57,7 +57,7 @@ op
   / "RLIKE"i { return "RLIKE"; }
 
 range "range"
-  = contig:contig range:range_range?
+  = contig:contig ":" range:range_range?
     { return { type: 'range', contig: contig, range: range } }
 
 contig
@@ -66,13 +66,20 @@ contig
   / "Y"
 
 range_range
-  = ":" start:(comma_num)? "-" end:(comma_num)?
+  = start:(comma_num)? "-" end:(comma_num)?
     { return {start: start, end: end} }
 
 comma_num "number with commas"
   = chars:[0-9,]+ { return parseInt(chars.join(',').replace(/,/g, ''), 10) }
 
 order_by
-  = "ORDER BY"i ws field:field ws order:("ASC"i / "DESC"i)?
-    { return { type: 'sort', field: field, order:order } }
+  = "ORDER BY"i ws field_list:order_field_list
+    { return { type: 'sort', fields: field_list } }
 
+order_field
+  = field:field ws order:("ASC"i / "DESC"i)?
+    { return {field: field, order: order} }
+
+order_field_list
+  = first:order_field rest:(ws "," ws v:order_field { return v; })*
+    { return [first].concat(rest); }