Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Merge branch 'sort-different-paths' into devel

  • Loading branch information...
commit 67da1aa8c39172df033970a2d132f6306160b55c 2 parents b25d765 + f986a7c
@glasser glasser authored
View
6 History.md
@@ -7,6 +7,12 @@
* minimongo: Support {a: {$regex: '', $options: 'i'}} #1874
+* minimongo: Fix sort implementation with multiple sort fields which each look
+ inside an array. eg, ensure that with sort key `{'a.x': 1, 'a.y': 1}`, the
+ document `{a: [{x: 0, y: 4}]}` sorts before
+ `{a: [{x: 0, y: 5}, {x: 1, y: 3}]}`, because the 3 should not be used as a
+ tie-breaker because it is not "next to" the tied 0s.
+
* Upgraded dependencies
- amplify: 1.1.2 (from 1.1.0)
View
8 packages/minimongo/minimongo.js
@@ -100,7 +100,7 @@ LocalCollection.Cursor = function (collection, selector, options) {
self._selectorId = undefined;
self.matcher = new Minimongo.Matcher(selector, self);
self.sorter = (self.matcher.hasGeoQuery() || options.sort) ?
- new Sorter(options.sort || []) : null;
+ new Minimongo.Sorter(options.sort || []) : null;
}
self.skip = options.skip;
self.limit = options.limit;
@@ -672,7 +672,7 @@ LocalCollection.prototype.update = function (selector, mod, options, callback) {
if (queryResult.result) {
// XXX Should we save the original even if mod ends up being a no-op?
self._saveOriginal(id, doc);
- self._modifyAndNotify(doc, mod, recomputeQids, queryResult.arrayIndex);
+ self._modifyAndNotify(doc, mod, recomputeQids, queryResult.arrayIndices);
++updateCount;
if (!options.multi)
return false; // break
@@ -738,7 +738,7 @@ LocalCollection.prototype.upsert = function (selector, mod, options, callback) {
};
LocalCollection.prototype._modifyAndNotify = function (
- doc, mod, recomputeQids, arrayIndex) {
+ doc, mod, recomputeQids, arrayIndices) {
var self = this;
var matched_before = {};
@@ -755,7 +755,7 @@ LocalCollection.prototype._modifyAndNotify = function (
var old_doc = EJSON.clone(doc);
- LocalCollection._modify(doc, mod, {arrayIndex: arrayIndex});
+ LocalCollection._modify(doc, mod, {arrayIndices: arrayIndices});
for (qid in self.queries) {
query = self.queries[qid];
View
103 packages/minimongo/minimongo_tests.js
@@ -268,30 +268,40 @@ Tinytest.add("minimongo - lookup", function (test) {
test.equal(lookupAX({a: {x: [1]}}), [{value: [1]}]);
test.equal(lookupAX({a: 5}), [{value: undefined}]);
test.equal(lookupAX({a: [{x: 1}, {x: [2]}, {y: 3}]}),
- [{value: 1, arrayIndex: 0},
- {value: [2], arrayIndex: 1},
- {value: undefined, arrayIndex: 2}]);
+ [{value: 1, arrayIndices: [0]},
+ {value: [2], arrayIndices: [1]},
+ {value: undefined, arrayIndices: [2]}]);
var lookupA0X = MinimongoTest.makeLookupFunction('a.0.x');
test.equal(lookupA0X({a: [{x: 1}]}), [
// From interpreting '0' as "0th array element".
- {value: 1, arrayIndex: 0},
+ {value: 1, arrayIndices: [0, 'x']},
// From interpreting '0' as "after branching in the array, look in the
// object {x:1} for a field named 0".
- {value: undefined, arrayIndex: 0}]);
+ {value: undefined, arrayIndices: [0]}]);
test.equal(lookupA0X({a: [{x: [1]}]}), [
- {value: [1], arrayIndex: 0},
- {value: undefined, arrayIndex: 0}]);
+ {value: [1], arrayIndices: [0, 'x']},
+ {value: undefined, arrayIndices: [0]}]);
test.equal(lookupA0X({a: 5}), [{value: undefined}]);
test.equal(lookupA0X({a: [{x: 1}, {x: [2]}, {y: 3}]}), [
// From interpreting '0' as "0th array element".
- {value: 1, arrayIndex: 0},
+ {value: 1, arrayIndices: [0, 'x']},
// From interpreting '0' as "after branching in the array, look in the
// object {x:1} for a field named 0".
- {value: undefined, arrayIndex: 0},
- {value: undefined, arrayIndex: 1},
- {value: undefined, arrayIndex: 2}
+ {value: undefined, arrayIndices: [0]},
+ {value: undefined, arrayIndices: [1]},
+ {value: undefined, arrayIndices: [2]}
]);
+
+ test.equal(
+ MinimongoTest.makeLookupFunction('w.x.0.z')({
+ w: [{x: [{z: 5}]}]}), [
+ // From interpreting '0' as "0th array element".
+ {value: 5, arrayIndices: [0, 0, 'x']},
+ // From interpreting '0' as "after branching in the array, look in the
+ // object {z:5} for a field named "0".
+ {value: undefined, arrayIndices: [0, 0]}
+ ]);
});
Tinytest.add("minimongo - selector_compiler", function (test) {
@@ -1683,7 +1693,13 @@ Tinytest.add("minimongo - ordering", function (test) {
{a: [5, [3, 19], 18]}
]);
-
+ // (0,4) < (0,5), so they go in this order. It's not correct to consider
+ // (0,3) as a sort key for the second document because they come from
+ // different a-branches.
+ verify({'a.x': 1, 'a.y': 1}, [
+ {a: [{x: 0, y: 4}]},
+ {a: [{x: 0, y: 5}, {x: 1, y: 3}]}
+ ]);
});
Tinytest.add("minimongo - sort", function (test) {
@@ -1774,6 +1790,69 @@ Tinytest.add("minimongo - array sort", function (test) {
_.range(c.find().count()));
});
+Tinytest.add("minimongo - sort keys", function (test) {
+ var keyListToObject = function (keyList) {
+ var obj = {};
+ _.each(keyList, function (key) {
+ obj[EJSON.stringify(key)] = true;
+ });
+ return obj;
+ };
+
+ var testKeys = function (sortSpec, doc, expectedKeyList) {
+ var expectedKeys = keyListToObject(expectedKeyList);
+ var sorter = new Minimongo.Sorter(sortSpec);
+
+ var actualKeyList = [];
+ sorter._generateKeysFromDoc(doc, function (key) {
+ actualKeyList.push(key);
+ });
+ var actualKeys = keyListToObject(actualKeyList);
+ test.equal(actualKeys, expectedKeys);
+ };
+
+ var testParallelError = function (sortSpec, doc) {
+ var sorter = new Minimongo.Sorter(sortSpec);
+ test.throws(function () {
+ sorter._generateKeysFromDoc(doc, function (){});
+ }, /parallel arrays/);
+ };
+
+ // Just non-array fields.
+ testKeys({'a.x': 1, 'a.y': 1},
+ {a: {x: 0, y: 5}},
+ [[0,5]]);
+
+ // Ensure that we don't get [0,3] and [1,5].
+ testKeys({'a.x': 1, 'a.y': 1},
+ {a: [{x: 0, y: 5}, {x: 1, y: 3}]},
+ [[0,5], [1,3]]);
+
+ // Ensure we can combine "array fields" with "non-array fields".
+ testKeys({'a.x': 1, 'a.y': 1, b: -1},
+ {a: [{x: 0, y: 5}, {x: 1, y: 3}], b: 42},
+ [[0,5,42], [1,3,42]]);
+ testKeys({b: -1, 'a.x': 1, 'a.y': 1},
+ {a: [{x: 0, y: 5}, {x: 1, y: 3}], b: 42},
+ [[42,0,5], [42,1,3]]);
+ testKeys({'a.x': 1, b: -1, 'a.y': 1},
+ {a: [{x: 0, y: 5}, {x: 1, y: 3}], b: 42},
+ [[0,42,5], [1,42,3]]);
+ testKeys({a: 1, b: 1},
+ {a: [1, 2, 3], b: 42},
+ [[1,42], [2,42], [3,42]]);
+
+ // Don't support multiple arrays at the same level.
+ testParallelError({a: 1, b: 1},
+ {a: [1, 2, 3], b: [42]});
+
+ // We are MORE STRICT than Mongo here; Mongo supports this!
+ // XXX support this too #NestedArraySort
+ testParallelError({'a.x': 1, 'a.y': 1},
+ {a: [{x: 1, y: [2, 3]},
+ {x: 2, y: [4, 5]}]});
+});
+
Tinytest.add("minimongo - binary search", function (test) {
var forwardCmp = function (a, b) {
return a - b;
View
11 packages/minimongo/modify.js
@@ -56,7 +56,7 @@ LocalCollection._modify = function (doc, mod, options) {
var target = findModTarget(newDoc, keyparts, {
noCreate: NO_CREATE_MODIFIERS[op],
forbidArray: (op === "$rename"),
- arrayIndex: options.arrayIndex
+ arrayIndices: options.arrayIndices
});
var field = keyparts.pop();
modFunc(target, field, arg, keypath, newDoc);
@@ -96,7 +96,8 @@ LocalCollection._modify = function (doc, mod, options) {
//
// if forbidArray is true, return null if the keypath goes through an array.
//
-// if options.arrayIndex is defined, use this for the (first) '$' in the path.
+// if options.arrayIndices is set, use its first element for the (first) '$' in
+// the path.
var findModTarget = function (doc, keyparts, options) {
options = options || {};
var usedArrayIndex = false;
@@ -118,11 +119,11 @@ var findModTarget = function (doc, keyparts, options) {
if (keypart === '$') {
if (usedArrayIndex)
throw MinimongoError("Too many positional (i.e. '$') elements");
- if (options.arrayIndex === undefined) {
+ if (!options.arrayIndices || !options.arrayIndices.length) {
throw MinimongoError("The positional operator did not find the " +
"match needed from the query");
}
- keypart = options.arrayIndex;
+ keypart = options.arrayIndices[0];
usedArrayIndex = true;
} else if (isNumericKey(keypart)) {
keypart = parseInt(keypart);
@@ -248,7 +249,7 @@ var MODIFIERS = {
// actually an extension of the Node driver, so it won't work
// server-side. Could be confusing!
// XXX is it correct that we don't do geo-stuff here?
- sortFunction = new Sorter(arg.$sort).getComparator();
+ sortFunction = new Minimongo.Sorter(arg.$sort).getComparator();
for (var i = 0; i < toPush.length; i++) {
if (LocalCollection._f._type(toPush[i]) !== 3) {
throw MinimongoError("$push like modifiers using $sort " +
View
124 packages/minimongo/selector.js
@@ -5,9 +5,9 @@
// - a "matcher" is its compiled form (whether a full Minimongo.Matcher
// object or one of the component lambdas that matches parts of it)
// - a "result object" is an object with a "result" field and maybe
-// distance and arrayIndex.
+// distance and arrayIndices.
// - a "branched value" is an object with a "value" field and maybe
-// "dontIterate" and "arrayIndex".
+// "dontIterate" and "arrayIndices".
// - a "document" is a top-level object that can be stored in a collection.
// - a "lookup function" is a function that takes in a document and returns
// an array of "branched values".
@@ -161,7 +161,7 @@ var compileValueSelector = function (valueSelector, matcher, isRoot) {
// Given an element matcher (which evaluates a single value), returns a branched
// value (which evaluates the element matcher on all the branches and returns a
-// more structured return value possibly including arrayIndex).
+// more structured return value possibly including arrayIndices).
var convertElementMatcherToBranchedMatcher = function (
elementMatcher, options) {
options = options || {};
@@ -175,18 +175,21 @@ var convertElementMatcherToBranchedMatcher = function (
ret.result = _.any(expanded, function (element) {
var matched = elementMatcher(element.value);
- // Special case for $elemMatch: it means "true, and use this arrayIndex if
- // I didn't already have one".
+ // Special case for $elemMatch: it means "true, and use this as an array
+ // index if I didn't already have one".
if (typeof matched === 'number') {
- if (element.arrayIndex === undefined)
- element.arrayIndex = matched;
+ // XXX This code dates from when we only stored a single array index
+ // (for the outermost array). Should we be also including deeper array
+ // indices from the $elemMatch match?
+ if (!element.arrayIndices)
+ element.arrayIndices = [matched];
matched = true;
}
- // If some element matched, and it's tagged with an array index, include
- // that index in our result object.
- if (matched && element.arrayIndex !== undefined)
- ret.arrayIndex = element.arrayIndex;
+ // If some element matched, and it's tagged with array indices, include
+ // those indices in our result object.
+ if (matched && element.arrayIndices)
+ ret.arrayIndices = element.arrayIndices;
return matched;
});
@@ -295,7 +298,7 @@ var LOGICAL_OPERATORS = {
subSelector, matcher, inElemMatch);
// Special case: if there is only one matcher, use it directly, *preserving*
- // any arrayIndex it returns.
+ // any arrayIndices it returns.
if (matchers.length === 1)
return matchers[0];
@@ -303,7 +306,7 @@ var LOGICAL_OPERATORS = {
var result = _.any(matchers, function (f) {
return f(doc).result;
});
- // $or does NOT set arrayIndex when it has multiple
+ // $or does NOT set arrayIndices when it has multiple
// sub-expressions. (Tested against MongoDB.)
return {result: result};
};
@@ -316,7 +319,7 @@ var LOGICAL_OPERATORS = {
var result = _.all(matchers, function (f) {
return !f(doc).result;
});
- // Never set arrayIndex, because we only match if nothing in particular
+ // Never set arrayIndices, because we only match if nothing in particular
// "matched" (and because this is consistent with MongoDB).
return {result: result};
};
@@ -353,7 +356,7 @@ var LOGICAL_OPERATORS = {
var invertBranchedMatcher = function (branchedMatcher) {
return function (branchValues) {
var invertMe = branchedMatcher(branchValues);
- // We explicitly choose to strip arrayIndex here: it doesn't make sense to
+ // We explicitly choose to strip arrayIndices here: it doesn't make sense to
// say "update the array element that does not match something", at least
// in mongo-land.
return {result: !invertMe.result};
@@ -473,10 +476,10 @@ var VALUE_OPERATORS = {
return;
result.result = true;
result.distance = curDistance;
- if (branch.arrayIndex === undefined)
- delete result.arrayIndex;
+ if (!branch.arrayIndices)
+ delete result.arrayIndices;
else
- result.arrayIndex = branch.arrayIndex;
+ result.arrayIndices = branch.arrayIndices;
});
return result;
};
@@ -504,6 +507,9 @@ var pointToArray = function (point) {
var makeInequality = function (cmpValueComparator) {
return function (operand) {
// Arrays never compare false with non-arrays for any inequality.
+ // XXX This was behavior we observed in pre-release MongoDB 2.5, but
+ // it seems to have been reverted.
+ // See https://jira.mongodb.org/browse/SERVER-11444
if (isArray(operand)) {
return function () {
return false;
@@ -686,7 +692,7 @@ var ELEMENT_OPERATORS = {
}
// XXX support $near in $elemMatch by propagating $distance?
if (subMatcher(arg).result)
- return i; // specially understood to mean "use my arrayIndex"
+ return i; // specially understood to mean "use as arrayIndices"
}
return false;
};
@@ -718,16 +724,33 @@ var ELEMENT_OPERATORS = {
// when there is a numeric index in the key, and ensures the
// perhaps-surprising MongoDB behavior where {'a.0': 5} does NOT
// match {a: [[5]]}.
-// - arrayIndex: if any array indexing was done during lookup (either
-// due to explicit numeric indices or implicit branching), this will
-// be the FIRST (outermost) array index used; it is undefined or absent
-// if no array index is used. (Make sure to check its value vs undefined,
-// not just for truth, since '0' is a legit array index!) This is used
-// to implement the '$' modifier feature.
+// - arrayIndices: if any array indexing was done during lookup (either due to
+// explicit numeric indices or implicit branching), this will be an array of
+// the array indices used, from outermost to innermost; it is falsey or
+// absent if no array index is used. If an explicit numeric index is used,
+// the index will be followed in arrayIndices by the string 'x'.
//
-// At the top level, you may only pass in a plain object or arraym.
+// Note: arrayIndices is used for two purposes. First, it is used to
+// implement the '$' modifier feature, which only ever looks at its first
+// element.
//
-// See the text 'minimongo - lookup' for some examples of what lookup functions
+// Second, it is used for sort key generation, which needs to be able to tell
+// the difference between different paths. Moreover, it needs to
+// differentiate between explicit and implicit branching, which is why
+// there's the somewhat hacky 'x' entry: this means that explicit and
+// implicit array lookups will have different full arrayIndices paths. (That
+// code only requires that different paths have different arrayIndices; it
+// doesn't actually "parse" arrayIndices. As an alternative, arrayIndices
+// could contain objects with flags like "implicit", but I think that only
+// makes the code surrounding them more complex.)
+//
+// (By the way, this field ends up getting passed around a lot without
+// cloning, so never mutate any arrayIndices field/var in this package!)
+//
+//
+// At the top level, you may only pass in a plain object or array.
+//
+// See the test 'minimongo - lookup' for some examples of what lookup functions
// return.
makeLookupFunction = function (key) {
var parts = key.split('.');
@@ -741,14 +764,17 @@ makeLookupFunction = function (key) {
var elideUnnecessaryFields = function (retVal) {
if (!retVal.dontIterate)
delete retVal.dontIterate;
- if (retVal.arrayIndex === undefined)
- delete retVal.arrayIndex;
+ if (retVal.arrayIndices && !retVal.arrayIndices.length)
+ delete retVal.arrayIndices;
return retVal;
};
// Doc will always be a plain object or an array.
// apply an explicit numeric index, an array.
- return function (doc, firstArrayIndex) {
+ return function (doc, arrayIndices) {
+ if (!arrayIndices)
+ arrayIndices = [];
+
if (isArray(doc)) {
// If we're being asked to do an invalid lookup into an array (non-integer
// or out-of-bounds), return no results (which is different from returning
@@ -756,10 +782,10 @@ makeLookupFunction = function (key) {
if (!(firstPartIsNumeric && firstPart < doc.length))
return [];
- // If this is the first array index we've seen, remember the index.
- // (Mongo doesn't support multiple uses of '$', at least not in 2.5.
- if (firstArrayIndex === undefined)
- firstArrayIndex = +firstPart;
+ // Remember that we used this array index. Include an 'x' to indicate that
+ // the previous index came from being considered as an explicit array
+ // index (not branching).
+ arrayIndices = arrayIndices.concat(+firstPart, 'x');
}
// Do our first lookup.
@@ -781,7 +807,7 @@ makeLookupFunction = function (key) {
return [elideUnnecessaryFields({
value: firstLevel,
dontIterate: isArray(doc) && isArray(firstLevel),
- arrayIndex: firstArrayIndex})];
+ arrayIndices: arrayIndices})];
}
// We need to dig deeper. But if we can't, because what we've found is not
@@ -794,7 +820,7 @@ makeLookupFunction = function (key) {
if (isArray(doc))
return [];
return [elideUnnecessaryFields({value: undefined,
- arrayIndex: firstArrayIndex})];
+ arrayIndices: arrayIndices})];
}
var result = [];
@@ -805,11 +831,11 @@ makeLookupFunction = function (key) {
// Dig deeper: look up the rest of the parts on whatever we've found.
// (lookupRest is smart enough to not try to do invalid lookups into
// firstLevel if it's an array.)
- appendToResult(lookupRest(firstLevel, firstArrayIndex));
+ appendToResult(lookupRest(firstLevel, arrayIndices));
// If we found an array, then in *addition* to potentially treating the next
- // part as a literal integer lookup, we should also "branch": try to do look
- // up the rest of the parts on each array element in parallel.
+ // part as a literal integer lookup, we should also "branch": try to look up
+ // the rest of the parts on each array element in parallel.
//
// In this case, we *only* dig deeper into array elements that are plain
// objects. (Recall that we only got this far if we have further to dig.)
@@ -822,7 +848,7 @@ makeLookupFunction = function (key) {
if (isPlainObject(branch)) {
appendToResult(lookupRest(
branch,
- firstArrayIndex === undefined ? arrayIndex : firstArrayIndex));
+ arrayIndices.concat(arrayIndex)));
}
});
}
@@ -843,17 +869,14 @@ expandArraysInBranches = function (branches, skipTheArrays) {
if (!(skipTheArrays && thisIsArray && !branch.dontIterate)) {
branchesOut.push({
value: branch.value,
- arrayIndex: branch.arrayIndex
+ arrayIndices: branch.arrayIndices
});
}
if (thisIsArray && !branch.dontIterate) {
_.each(branch.value, function (leaf, i) {
branchesOut.push({
value: leaf,
- // arrayIndex always defaults to the outermost array, but if we didn't
- // need to use an array to get to this branch, we mark the index we
- // just used as the arrayIndex.
- arrayIndex: branch.arrayIndex === undefined ? i : branch.arrayIndex
+ arrayIndices: (branch.arrayIndices || []).concat(i)
});
});
}
@@ -881,7 +904,6 @@ var andSomeMatchers = function (subMatchers) {
return subMatchers[0];
return function (docOrBranches) {
- // XXX arrayIndex!
var ret = {};
ret.result = _.all(subMatchers, function (f) {
var subResult = f(docOrBranches);
@@ -893,11 +915,11 @@ var andSomeMatchers = function (subMatchers) {
&& ret.distance === undefined) {
ret.distance = subResult.distance;
}
- // Similarly, propagate arrayIndex from sub-matchers... but to match
- // MongoDB behavior, this time the *last* sub-matcher with an arrayIndex
+ // Similarly, propagate arrayIndices from sub-matchers... but to match
+ // MongoDB behavior, this time the *last* sub-matcher with arrayIndices
// wins.
- if (subResult.result && subResult.arrayIndex !== undefined) {
- ret.arrayIndex = subResult.arrayIndex;
+ if (subResult.result && subResult.arrayIndices) {
+ ret.arrayIndices = subResult.arrayIndices;
}
return subResult.result;
});
@@ -905,7 +927,7 @@ var andSomeMatchers = function (subMatchers) {
// If we didn't actually match, forget any extra metadata we came up with.
if (!ret.result) {
delete ret.distance;
- delete ret.arrayIndex;
+ delete ret.arrayIndices;
}
return ret;
};
View
299 packages/minimongo/sort.js
@@ -11,21 +11,21 @@
// first object comes first in order, 1 if the second object comes
// first, or 0 if neither object comes before the other.
-Sorter = function (spec) {
+Minimongo.Sorter = function (spec) {
var self = this;
- var sortSpecParts = self._sortSpecParts = [];
+ self._sortSpecParts = [];
if (spec instanceof Array) {
for (var i = 0; i < spec.length; i++) {
if (typeof spec[i] === "string") {
- sortSpecParts.push({
+ self._sortSpecParts.push({
path: spec[i],
lookup: makeLookupFunction(spec[i]),
ascending: true
});
} else {
- sortSpecParts.push({
+ self._sortSpecParts.push({
path: spec[i][0],
lookup: makeLookupFunction(spec[i][0]),
ascending: spec[i][1] !== "desc"
@@ -34,7 +34,7 @@ Sorter = function (spec) {
}
} else if (typeof spec === "object") {
for (var key in spec) {
- sortSpecParts.push({
+ self._sortSpecParts.push({
path: key,
lookup: makeLookupFunction(key),
ascending: spec[key] >= 0
@@ -44,89 +44,234 @@ Sorter = function (spec) {
throw Error("Bad sort specification: " + JSON.stringify(spec));
}
- // reduceValue takes in all the possible values for the sort key along various
- // branches, and returns the min or max value (according to the bool
- // findMin). Each value can itself be an array, and we look at its values
- // too. (ie, we do a single level of flattening on branchValues, then find the
- // min/max.)
- //
- // XXX This is actually wrong! In fact, the whole attempt to compile sort
- // functions independently of selectors is wrong. In MongoDB, if you have
- // documents {_id: 'x', a: [1, 10]} and {_id: 'y', a: [5, 15]}, then
- // C.find({}, {sort: {a: 1}}) puts x before y (1 comes before 5). But
- // C.find({a: {$gt: 3}}, {sort: {a: 1}}) puts y before x (1 does not match
- // the selector, and 5 comes before 10).
+ self._keyComparator = composeComparators(
+ _.map(self._sortSpecParts, function (spec, i) {
+ return self._keyFieldComparator(i);
+ }));
+};
+
+// In addition to these methods, sorter_project.js defines combineIntoProjection
+// on the server only.
+_.extend(Minimongo.Sorter.prototype, {
+ getComparator: function (options) {
+ var self = this;
+
+ // If we have no distances, just use the comparator from the source
+ // specification (which defaults to "everything is equal".
+ if (!options || !options.distances) {
+ return self._getBaseComparator();
+ }
+
+ var distances = options.distances;
+
+ // Return a comparator which first tries the sort specification, and if that
+ // says "it's equal", breaks ties using $near distances.
+ return composeComparators([self._getBaseComparator(), function (a, b) {
+ if (!distances.has(a._id))
+ throw Error("Missing distance for " + a._id);
+ if (!distances.has(b._id))
+ throw Error("Missing distance for " + b._id);
+ return distances.get(a._id) - distances.get(b._id);
+ }]);
+ },
+
+ _getPaths: function () {
+ var self = this;
+ return _.pluck(self._sortSpecParts, 'path');
+ },
+
+ // Finds the minimum key from the doc, according to the sort specs. (We say
+ // "minimum" here but this is with respect to the sort spec, so "descending"
+ // sort fields mean we're finding the max for that field.)
//
- // The way this works is pretty subtle! For example, if the documents are
- // instead {_id: 'x', a: [{x: 1}, {x: 10}]}) and
- // {_id: 'y', a: [{x: 5}, {x: 15}]}),
- // then C.find({'a.x': {$gt: 3}}, {sort: {'a.x': 1}}) and
- // C.find({a: {$elemMatch: {x: {$gt: 3}}}}, {sort: {'a.x': 1}})
- // both follow this rule (y before x). ie, you do have to apply this
- // through $elemMatch.
- var reduceValue = function (branchValues, findMin) {
- // Expand any leaf arrays that we find, and ignore those arrays themselves.
- branchValues = expandArraysInBranches(branchValues, true);
- var reduced = undefined;
- var first = true;
- // Iterate over all the values found in all the branches, and if a value is
- // an array itself, iterate over the values in the array separately.
- _.each(branchValues, function (branchValue) {
- if (first) {
- reduced = branchValue.value;
- first = false;
- } else {
- // Compare the value we found to the value we found so far, saving it
- // if it's less (for an ascending sort) or more (for a descending
- // sort).
- var cmp = LocalCollection._f._cmp(reduced, branchValue.value);
- if ((findMin && cmp > 0) || (!findMin && cmp < 0))
- reduced = branchValue.value;
+ // Note that this is NOT "find the minimum value of the first field, the
+ // minimum value of the second field, etc"... it's "choose the
+ // lexicographically minimum value of the key vector, allowing only keys which
+ // you can find along the same paths". ie, for a doc {a: [{x: 0, y: 5}, {x:
+ // 1, y: 3}]} with sort spec {'a.x': 1, 'a.y': 1}, the only keys are [0,5] and
+ // [1,3], and the minimum key is [0,5]; notably, [0,3] is NOT a key.
+ _getMinKeyFromDoc: function (doc) {
+ var self = this;
+ var minKey = null;
+
+ self._generateKeysFromDoc(doc, function (key) {
+ // XXX This is actually wrong! In fact, the whole attempt to compile sort
+ // functions independently of selectors is wrong. In MongoDB, if you
+ // have documents {_id: 'x', a: [1, 10]} and {_id: 'y', a: [5, 15]},
+ // then C.find({}, {sort: {a: 1}}) puts x before y (1 comes before 5).
+ // But C.find({a: {$gt: 3}}, {sort: {a: 1}}) puts y before x (1 does
+ // not match the selector, and 5 comes before 10).
+ //
+ // The way this works is pretty subtle! For example, if the documents
+ // are instead {_id: 'x', a: [{x: 1}, {x: 10}]}) and
+ // {_id: 'y', a: [{x: 5}, {x: 15}]}),
+ // then C.find({'a.x': {$gt: 3}}, {sort: {'a.x': 1}}) and
+ // C.find({a: {$elemMatch: {x: {$gt: 3}}}}, {sort: {'a.x': 1}})
+ // both follow this rule (y before x). (ie, you do have to apply this
+ // through $elemMatch.)
+ //
+ // So we ought to skip keys here that don't work for the selector, but
+ // we don't do that yet.
+
+ if (minKey === null) {
+ minKey = key;
+ return;
+ }
+ if (self._compareKeys(key, minKey) < 0) {
+ minKey = key;
}
});
- return reduced;
- };
- var comparators = _.map(sortSpecParts, function (specPart) {
- return function (a, b) {
- var aValue = reduceValue(specPart.lookup(a), specPart.ascending);
- var bValue = reduceValue(specPart.lookup(b), specPart.ascending);
- var compare = LocalCollection._f._cmp(aValue, bValue);
- return specPart.ascending ? compare : -compare;
+ if (minKey === null)
+ throw Error("sort selector found no keys in doc?");
+ return minKey;
+ },
+
+ // Iterates over each possible "key" from doc (ie, over each branch), calling
+ // 'cb' with the key.
+ _generateKeysFromDoc: function (doc, cb) {
+ var self = this;
+
+ if (self._sortSpecParts.length === 0)
+ throw new Error("can't generate keys without a spec");
+
+ // maps index -> ({'' -> value} or {path -> value})
+ var valuesByIndexAndPath = [];
+
+ var pathFromIndices = function (indices) {
+ return indices.join(',') + ',';
};
- });
- self._baseComparator = composeComparators(comparators);
-};
+ var knownPaths = null;
-Sorter.prototype.getComparator = function (options) {
- var self = this;
+ _.each(self._sortSpecParts, function (spec, whichField) {
+ // Expand any leaf arrays that we find, and ignore those arrays
+ // themselves. (We never sort based on an array itself.)
+ var branches = expandArraysInBranches(spec.lookup(doc), true);
- // If we have no distances, just use the comparator from the source
- // specification (which defaults to "everything is equal".
- if (!options || !options.distances) {
- return self._baseComparator;
- }
+ // If there are no values for a key (eg, key goes to an empty array),
+ // pretend we found one null value.
+ if (!branches.length)
+ branches = [{value: null}];
- var distances = options.distances;
-
- // Return a comparator which first tries the sort specification, and if that
- // says "it's equal", breaks ties using $near distances.
- return composeComparators([self._baseComparator, function (a, b) {
- if (!distances.has(a._id))
- throw Error("Missing distance for " + a._id);
- if (!distances.has(b._id))
- throw Error("Missing distance for " + b._id);
- return distances.get(a._id) - distances.get(b._id);
- }]);
-};
+ var usedPaths = false;
+ valuesByIndexAndPath[whichField] = {};
+ _.each(branches, function (branch) {
+ if (!branch.arrayIndices) {
+ // If there are no array indices for a branch, then it must be the
+ // only branch, because the only thing that produces multiple branches
+ // is the use of arrays.
+ if (branches.length > 1)
+ throw Error("multiple branches but no array used?");
+ valuesByIndexAndPath[whichField][''] = branch.value;
+ return;
+ }
-Sorter.prototype._getPaths = function () {
- var self = this;
- return _.pluck(self._sortSpecParts, 'path');
-};
+ usedPaths = true;
+ var path = pathFromIndices(branch.arrayIndices);
+ if (_.has(valuesByIndexAndPath[whichField], path))
+ throw Error("duplicate path: " + path);
+ valuesByIndexAndPath[whichField][path] = branch.value;
+
+ // If two sort fields both go into arrays, they have to go into the
+ // exact same arrays and we have to find the same paths. This is
+ // roughly the same condition that makes MongoDB throw this strange
+ // error message. eg, the main thing is that if sort spec is {a: 1,
+ // b:1} then a and b cannot both be arrays.
+ //
+ // (In MongoDB it seems to be OK to have {a: 1, 'a.x.y': 1} where 'a'
+ // and 'a.x.y' are both arrays, but we don't allow this for now.
+ // #NestedArraySort
+ // XXX achieve full compatibility here
+ if (knownPaths && !_.has(knownPaths, path)) {
+ throw Error("cannot index parallel arrays");
+ }
+ });
+
+ if (knownPaths) {
+ // Similarly to above, paths must match everywhere, unless this is a
+ // non-array field.
+ if (!_.has(valuesByIndexAndPath[whichField], '') &&
+ _.size(knownPaths) !== _.size(valuesByIndexAndPath[whichField])) {
+ throw Error("cannot index parallel arrays!");
+ }
+ } else if (usedPaths) {
+ knownPaths = {};
+ _.each(valuesByIndexAndPath[whichField], function (x, path) {
+ knownPaths[path] = true;
+ });
+ }
+ });
+
+ if (!knownPaths) {
+ // Easy case: no use of arrays.
+ var soleKey = _.map(valuesByIndexAndPath, function (values) {
+ if (!_.has(values, ''))
+ throw Error("no value in sole key case?");
+ return values[''];
+ });
+ cb(soleKey);
+ return;
+ }
-Minimongo.Sorter = Sorter;
+ _.each(knownPaths, function (x, path) {
+ var key = _.map(valuesByIndexAndPath, function (values) {
+ if (_.has(values, ''))
+ return values[''];
+ if (!_.has(values, path))
+ throw Error("missing path?");
+ return values[path];
+ });
+ cb(key);
+ });
+ },
+
+ // Takes in two keys: arrays whose lengths match the number of spec
+ // parts. Returns negative, 0, or positive based on using the sort spec to
+ // compare fields.
+ _compareKeys: function (key1, key2) {
+ var self = this;
+ if (key1.length !== self._sortSpecParts.length ||
+ key2.length !== self._sortSpecParts.length) {
+ throw Error("Key has wrong length");
+ }
+
+ return self._keyComparator(key1, key2);
+ },
+
+ // Given an index 'i', returns a comparator that compares two key arrays based
+ // on field 'i'.
+ _keyFieldComparator: function (i) {
+ var self = this;
+ var invert = !self._sortSpecParts[i].ascending;
+ return function (key1, key2) {
+ var compare = LocalCollection._f._cmp(key1[i], key2[i]);
+ if (invert)
+ compare = -compare;
+ return compare;
+ };
+ },
+
+ // Returns a comparator that represents the sort specification (but not
+ // including a possible geoquery distance tie-breaker).
+ _getBaseComparator: function () {
+ var self = this;
+
+ // If we're only sorting on geoquery distance and no specs, just say
+ // everything is equal.
+ if (!self._sortSpecParts.length) {
+ return function (doc1, doc2) {
+ return 0;
+ };
+ }
+
+ return function (doc1, doc2) {
+ var key1 = self._getMinKeyFromDoc(doc1);
+ var key2 = self._getMinKeyFromDoc(doc2);
+ return self._compareKeys(key1, key2);
+ };
+ }
+});
// Given an array of comparators
// (functions (a,b)->(negative or positive or zero)), returns a single
View
3  packages/minimongo/sorter_projection.js
@@ -1,6 +1,5 @@
-Sorter.prototype.combineIntoProjection = function (projection) {
+Minimongo.Sorter.prototype.combineIntoProjection = function (projection) {
var self = this;
var specPaths = Minimongo._pathsElidingNumericKeys(self._getPaths());
return combineImportantPathsIntoProjection(specPaths, projection);
};
-
Please sign in to comment.
Something went wrong with that request. Please try again.