Merge pull request #81 from RiveraGroup/feature/mongo_2.6.5_expressions_Let

Feature/mongo 2.6.5 expressions let

lib/pipeline/expressions/LetExpression.js

@@ -1,119 +1,126 @@
 "use strict";
 
 var LetExpression = module.exports = function LetExpression(vars, subExpression){
-	//if (arguments.length !== 2) throw new Error("Two args expected");
+	if (arguments.length !== 2) throw new Error(klass.name + ": expected args: vars, subExpression");
 	this._variables = vars;
 	this._subExpression = subExpression;
 }, klass = LetExpression, Expression = require("./Expression"), base = Expression, proto = klass.prototype = Object.create(base.prototype, {constructor:{value:klass}});
 
-Expression.registerExpression("$let", LetExpression.parse);
 
-// DEPENDENCIES
-var Variables = require("./Variables"),
-	VariablesParseState = require("./VariablesParseState");
+var Value = require("../Value"),
+	Variables = require("./Variables");
 
-// PROTOTYPE MEMBERS
 
+function NameAndExpression(name, expr){
+	this.name = name;
+	this.expression = expr;
+}
 
-proto.parse = function parse(expr, vpsIn){
-	if(!("$let" in expr)) {
-		throw new Error("Tried to create a $let with something other than let. Looks like your parse map went all funny.");
-	}
 
-	if(typeof(expr.$let) !== 'object' || (expr.$let instanceof Array)) {
-		throw new Error("$let only supports an object as its argument: 16874");
-	}
+klass.parse = function parse(expr, vpsIn){
 
-	var args = expr.$let,
-		varsElem = args.vars,
-		inElem = args['in']; // args.in; ??
-
-	//NOTE: DEVIATION FROM MONGO: 1. These if statements are in a loop in the c++ version,
-	// 2. 'vars' and 'in' are each mandatory here. in the c++ code you only need one of the two.
-	// 3. Below, we croak if there are more than 2 arguments.  The original does not have this limitation, specifically.
-	// Upon further review, I think our code is more accurate.  The c++ code will accept if there are multiple 'in'
-	// or 'var' values. The previous ones will be overwritten by newer ones.
-	//
-	// Final note - I think this code is fine.
-	//
-	if(!varsElem) {
-		throw new Error("Missing 'vars' parameter to $let: 16876");
-	}
-	if(!inElem) {
-		throw new Error("Missing 'in' parameter to $let: 16877");
-	}
+	// if (!(exprFieldName === "$let")) throw new Error("Assertion failure"); //NOTE: DEVIATION FROM MONGO: we do not have exprFieldName here
 
-	// Should this be !== 2?  Why would we have fewer than 2 arguments?  Why do we even care what the length of the
-	// array is? It may be an optimization of sorts. But what we're really wanting here is, 'If any keys are not "in"
-	// or "vars" then we need to bugcheck.'
-	if(Object.keys(args).length > 2) {
-		var bogus = Object.keys(args).filter(function(x) {return !(x === 'in' || x === 'vars');});
-		throw new Error("Unrecognized parameter to $let: " + bogus.join(",") + "- 16875");
-	}
+	if (Value.getType(expr) !== "Object")
+		throw new Error("$let only supports an object as it's argument; uassert code 16874");
+	var args = expr;
 
-	var vpsSub = new VariablesParseState(vpsIn),
-		vars = {};
+	// varsElem must be parsed before inElem regardless of BSON order.
+	var varsElem,
+		inElem;
+	for (var argFieldName in args) {
+		var arg = args[argFieldName];
+		if (argFieldName === "vars") {
+			varsElem = arg;
+		} else if (argFieldName === "in") {
+			inElem = arg;
+		} else {
+			throw new Error("Unrecognized parameter to $let: " + argFieldName + "; uasserted code 16875");
+		}
+	}
 
-	for(var varName in varsElem) {
+	if (!varsElem)
+		throw new Error("Missing 'vars' parameter to $let; uassert code 16876");
+	if (!inElem)
+		throw new Error("Missing 'in' parameter to $let; uassert code 16877");
+
+	// parse "vars"
+	var vpsSub = vpsIn, // vpsSub gets our vars, vpsIn doesn't.
+		vars = {}; // using like a VariableMap
+	if (Value.getType(varsElem) !== "Object") //NOTE: emulate varsElem.embeddedObjectUserCheck()
+		throw new Error("invalid parameter: expected an object (vars); uasserted code 10065");
+	for (var varName in varsElem) {
+		var varElem = varsElem[varName];
 		Variables.uassertValidNameForUserWrite(varName);
 		var id = vpsSub.defineVariable(varName);
 
-		vars[id] = {};
-		vars[id][varName] = Expression.parseOperand(varsElem, vpsIn);
+		vars[id] = new NameAndExpression(varName,
+			Expression.parseOperand(varElem, vpsIn)); // only has outer vars
 	}
 
-	var subExpression = Expression.parseOperand(inElem, vpsSub);
+	// parse "in"
+	var subExpression = Expression.parseOperand(inElem, vpsSub); // has our vars
+
 	return new LetExpression(vars, subExpression);
 };
 
+
 proto.optimize = function optimize() {
-	if(this._variables.empty()) {
+	if (Object.keys(this._variables).length === 0) {
+		// we aren't binding any variables so just return the subexpression
 		return this._subExpression.optimize();
 	}
 
-	for(var id in this._variables){
-		for(var name in this._variables[id]) {
-			//NOTE: DEVIATION FROM MONGO: This is actually ok. The c++ code does this with a single map. The js structure
-			// is nested objects.
-			this._variables[id][name] = this._variables[id][name].optimize();
-		}
+	for (var id in this._variables) {
+		this._variables[id].expression = this._variables[id].expression.optimize();
 	}
 
+	// TODO be smarter with constant "variables"
 	this._subExpression = this._subExpression.optimize();
 
 	return this;
 };
 
+
 proto.serialize = function serialize(explain) {
 	var vars = {};
-	for(var id in this._variables) {
-		for(var name in this._variables[id]) {
-			vars[name] = this._variables[id][name];
-		}
+	for (var id in this._variables) {
+		vars[this._variables[id].name] = this._variables[id].expression.serialize(explain);
 	}
 
-	return {$let: {vars:vars, 'in':this._subExpression.serialize(explain)}};
+	return {
+		$let: {
+			vars: vars,
+			in : this._subExpression.serialize(explain)
+		}
+	};
 };
 
+
 proto.evaluateInternal = function evaluateInternal(vars) {
-	for(var id in this._variables) {
-		for(var name in this._variables[id]) {
-			vars.setValue(id, this._variables[id][name]);
-		}
+	for (var id in this._variables) {
+		var itFirst = +id, //NOTE: using the unary + to coerce it to a Number
+			itSecond = this._variables[itFirst];
+		// It is guaranteed at parse-time that these expressions don't use the variable ids we
+		// are setting
+		vars.setValue(itFirst,
+			itSecond.expression.evaluateInternal(vars));
 	}
 
 	return this._subExpression.evaluateInternal(vars);
 };
 
+
 proto.addDependencies = function addDependencies(deps, path){
-	for(var id in this._variables) {
-		for(var name in this._variables[id]) {
-			this._variables[id][name].addDependencies(deps);
-		}
+	for (var id in this._variables) {
+		var itFirst = +id, //NOTE: using the unary + to coerce it to a Number
+			itSecond = this._variables[itFirst];
+			itSecond.expression.addDependencies(deps);
 	}
-	this._subExpression.addDependencies(deps, path);
-	return deps; //NOTE: DEVIATION FROM MONGO: The c++ version does not return a value. We seem to use the returned value
-					// (or something from a different method named
-					// addDependencies) in many places.
 
+	// TODO be smarter when CURRENT is a bound variable
+	this._subExpression.addDependencies(deps);
 };
+
+
+Expression.registerExpression("$let", LetExpression.parse);

lib/pipeline/expressions/ObjectExpression.js

@@ -49,7 +49,7 @@ proto.optimize = function optimize() {
 		if (!this._expressions.hasOwnProperty(key)) continue;
 		var expr = this._expressions[key];
 		if (expr)
-			expr.optimize();
+			this._expressions[key] = expr.optimize();
 	}
 	return this;
 };

test/lib/pipeline/expressions/AddExpression_test.js

@@ -238,7 +238,6 @@ exports.AddExpression = {
 			assert.strictEqual(expr.operands.length, 2, "should optimize operands away");
 			assert(expr.operands[0] instanceof FieldPathExpression);
 			assert(expr.operands[1] instanceof ConstantExpression);
-			debugger
 			assert.strictEqual(expr.operands[1].evaluate(), 1 + 2 + 3 + 4 + 5 + 6);
 		},
 

test/lib/pipeline/expressions/ConcatExpression_test.js

@@ -80,7 +80,7 @@ exports.ConcatExpression = {
 		},
 
 		"should throw if an operand is a boolean": function() {
-			var expr = Expression.parseOperand({$concat:["my","$a"]}, this.vps)
+			var expr = Expression.parseOperand({$concat:["my","$a"]}, this.vps);
 			assert.throws(function() {
 				expr.evaluate({a:true});
 			});

test/lib/pipeline/expressions/LetExpression_test.js

@@ -0,0 +1,197 @@
+"use strict";
+var assert = require("assert"),
+	DepsTracker = require("../../../../lib/pipeline/DepsTracker"),
+	LetExpression = require("../../../../lib/pipeline/expressions/LetExpression"),
+	ConstantExpression = require("../../../../lib/pipeline/expressions/ConstantExpression"),
+	MultiplyExpression = require("../../../../lib/pipeline/expressions/MultiplyExpression"), //jshint ignore:line
+	AddExpression = require("../../../../lib/pipeline/expressions/AddExpression"), //jshint ignore:line
+	CondExpression = require("../../../../lib/pipeline/expressions/CondExpression"), //jshint ignore:line
+	FieldPathExpression = require("../../../../lib/pipeline/expressions/FieldPathExpression"), //jshint ignore:line
+	VariablesParseState = require("../../../../lib/pipeline/expressions/VariablesParseState"),
+	Variables = require("../../../../lib/pipeline/expressions/Variables"),
+	VariablesIdGenerator = require("../../../../lib/pipeline/expressions/VariablesIdGenerator"),
+	Expression = require("../../../../lib/pipeline/expressions/Expression");
+
+// Mocha one-liner to make these tests self-hosted
+if(!module.parent)return(require.cache[__filename]=null,(new(require("mocha"))({ui:"exports",reporter:"spec",grep:process.env.TEST_GREP})).addFile(__filename).run(process.exit));
+
+exports.LetExpression = {
+
+	beforeEach: function() {
+		this.vps = new VariablesParseState(new VariablesIdGenerator());
+	},
+
+	"constructor()": {
+
+		"should throw an Error when constructing without args": function() {
+			assert.throws(function() {
+				new LetExpression();
+			});
+		},
+
+		"should throw Error when constructing with one arg": function() {
+			assert.throws(function() {
+				new LetExpression(1);
+			});
+		},
+
+		"should not throw when constructing with two args": function() {
+			assert.doesNotThrow(function() {
+				new LetExpression(1, 2);
+			});
+		},
+
+	},
+
+	"#parse()": {
+
+		"should throw if $let isn't in expr": function() {
+			var self = this;
+			assert.throws(function() {
+				Expression.parseOperand({$xlet: ['$$a', 1]}, self.vps);
+			}, /15999/);
+		},
+
+		"should throw if the $let expression isn't an object": function() {
+			var self = this;
+			assert.throws(function() {
+				Expression.parseOperand({$let: "this is not an object"}, self.vps);
+			}, /16874/);
+		},
+
+		"should throw if the $let expression is an array": function() {
+			var self = this;
+			assert.throws(function() {
+				Expression.parseOperand({$let: [1, 2, 3]}, self.vps);
+			}, /16874/);
+		},
+
+		"should throw if there is no vars parameter to $let": function() {
+			var self = this;
+			assert.throws(function() {
+				Expression.parseOperand({$let: {vars: undefined}}, self.vps);
+			}, /16876/);
+		},
+
+		"should throw if there is no input parameter to $let": function() {
+			var self = this;
+			assert.throws(function() {
+				Expression.parseOperand({$let: {vars: 1, in: undefined}}, self.vps);
+			}, /16877/);
+		},
+
+		"should throw if any of the arguments to $let are not 'in' or 'vars'": function() {
+			var self = this;
+			assert.throws(function() {
+				Expression.parseOperand({$let: {vars: 1, in: 2, zoot:3}}, self.vps);
+			}, /16875/);
+		},
+
+		"should return a Let expression": function() {
+			var x = Expression.parseOperand({$let: {vars: {a:{$const:123}}, in: 2}}, this.vps);
+			assert(x instanceof LetExpression);
+			assert(x._subExpression instanceof ConstantExpression);
+			assert.strictEqual(x._subExpression.getValue(), 2);
+			assert(x._variables[0].expression instanceof ConstantExpression);
+			assert.strictEqual(x._variables[0].expression.getValue(), 123);
+		},
+
+		"should show we collect multiple vars": function() {
+			var x = Expression.parseOperand({$let: {vars: {a:{$const:1}, b:{$const:2}, c:{$const:3}}, in: 2}}, this.vps);
+			assert.strictEqual(x._variables[0].expression.getValue(), 1);
+			assert.strictEqual(x._variables[1].expression.getValue(), 2);
+			assert.strictEqual(x._variables[2].expression.getValue(), 3);
+		},
+
+	},
+
+	"#optimize()": {
+
+		beforeEach: function() {
+			this.testInOpt = function(expr, expected) {
+				assert(expr._subExpression instanceof ConstantExpression, "should have $const subexpr");
+				assert.strictEqual(expr._subExpression.operands.length, 0);
+				assert.strictEqual(expr._subExpression.getValue(), expected);
+			};
+			this.testVarOpt = function(expr, expected) {
+				var varExpr = expr._variables[0].expression;
+				assert(varExpr instanceof ConstantExpression, "should have $const first var");
+				assert.strictEqual(varExpr.getValue(), expected);
+			};
+		},
+
+		"should optimize to subexpression if no variables": function() {
+			var x = Expression.parseOperand({$let:{vars:{}, in:{$multiply:[2,3]}}}, this.vps).optimize();
+			assert(x instanceof ConstantExpression, "should become $const");
+			assert.strictEqual(x.getValue(), 6);
+		},
+
+		"should optimize variables": function() {
+			var x = Expression.parseOperand({$let:{vars:{a:{$multiply:[5,4]}}, in:{$const:6}}}, this.vps).optimize();
+			this.testVarOpt(x, 20);
+		},
+
+		"should optimize subexpressions if there are variables": function() {
+			var x = Expression.parseOperand({$let:{vars:{a:{$multiply:[5,4]}}, in: {$multiply:[2,3]}}}, this.vps).optimize();
+			this.testInOpt(x, 6);
+			this.testVarOpt(x, 20);
+		},
+
+	},
+
+	"#serialize()": {
+
+		"should serialize variables and the subexpression": function() {
+			var s = Expression.parseOperand({$let: {vars: {a:{$const:1}, b:{$const:2}}, in: {$multiply: [2,3]}}}, this.vps).optimize().serialize("zoot");
+			var expected = {$let:{vars:{a:{$const:1},b:{$const:2}},in:{$const:6}}};
+			assert.deepEqual(s, expected);
+		},
+
+	},
+
+	"#evaluate()": {
+
+		"should perform the evaluation for variables and the subexpression": function() {
+			var x = Expression.parseOperand({$let: {vars: {a: '$in1', b: '$in2'}, in: { $multiply: ["$$a", "$$b"] }}}, this.vps).optimize();
+			var	y = x.evaluate(new Variables(10, {in1: 6, in2: 7}));
+			assert.equal(y, 42);
+		},
+
+	},
+
+	"#addDependencies()": {
+
+		"should add dependencies": function() {
+			var expr = Expression.parseOperand({$let: {vars: {a: {$multiply:['$a','$b']}}, in: {$multiply: ['$c','$d']}}}, this.vps);
+			var deps = new DepsTracker();
+			expr.addDependencies(deps);
+			assert.equal(Object.keys(deps.fields).length, 4);
+			assert('a' in deps.fields);
+			assert('b' in deps.fields);
+			assert('c' in deps.fields);
+			assert('d' in deps.fields);
+			assert.strictEqual(deps.needWholeDocument, false);
+			assert.strictEqual(deps.needTextScore, false);
+		},
+
+	},
+
+	"The Gauntlet": {
+
+		"example from http://docs.mongodb.org/manual/reference/operator/aggregation/let/": function() {
+			var x = Expression.parseOperand(
+				{$let: { vars: { total: { $add: [ '$price', '$tax' ] },	discounted: { $cond: { if: '$applyDiscount', then: 0.9, else: 1 } }}, in: { $multiply: [ '$$total', '$$discounted' ] }}},
+				this.vps).optimize();
+			var y;
+			y = x.evaluate(new Variables(10, {price: 90, tax: 0.05}));
+			assert.equal(y, 90.05);
+			y = x.evaluate(new Variables(10, {price: 90, tax: 0.05, applyDiscount: 1}));
+			assert.equal(y, 90.05 * 0.9);
+		},
+
+	},
+
+};
+
+
+if (!module.parent)(new (require("mocha"))()).ui("exports").reporter("spec").addFile(__filename).run(process.exit);