From c55eb86ef46ee7aede3b1e2a5d184a7df4bfb5b5 Mon Sep 17 00:00:00 2001 From: Kyle Suarez Date: Tue, 27 Jun 2017 12:00:57 -0400 Subject: SERVER-29618 fix serialization of geo match expressions Explicitly blacklist lookup_absorb_match from aggregation_sharded_collections_passthrough. (cherry picked from commit 18bc61d22123da5897d275eb92576522a1bab4de) --- ...aggregation_sharded_collections_passthrough.yml | 2 + .../sources/lookup/lookup_absorb_match.js | 97 ++++++++++++++++++++++ src/mongo/db/matcher/expression_geo.cpp | 14 +++- src/mongo/db/matcher/expression_geo.h | 10 +-- src/mongo/db/matcher/expression_parser_geo.cpp | 14 +--- 5 files changed, 116 insertions(+), 21 deletions(-) create mode 100644 jstests/aggregation/sources/lookup/lookup_absorb_match.js diff --git a/buildscripts/resmokeconfig/suites/aggregation_sharded_collections_passthrough.yml b/buildscripts/resmokeconfig/suites/aggregation_sharded_collections_passthrough.yml index d179184abc8..fa367432dc5 100644 --- a/buildscripts/resmokeconfig/suites/aggregation_sharded_collections_passthrough.yml +++ b/buildscripts/resmokeconfig/suites/aggregation_sharded_collections_passthrough.yml @@ -24,6 +24,8 @@ selector: - jstests/aggregation/bugs/cursor_timeout.js - jstests/aggregation/bugs/lookup_unwind_getmore.js - jstests/aggregation/bugs/lookup_unwind_killcursor.js + # The following tests assume that accessed collections are unsharded. + - jstests/aggregation/sources/lookup/lookup_absorb_match.js executor: js_test: diff --git a/jstests/aggregation/sources/lookup/lookup_absorb_match.js b/jstests/aggregation/sources/lookup/lookup_absorb_match.js new file mode 100644 index 00000000000..b00e86aaa98 --- /dev/null +++ b/jstests/aggregation/sources/lookup/lookup_absorb_match.js @@ -0,0 +1,97 @@ +/** + * Tests that a $match with a geo expression still returns the correct results if it has been + * absorbed by a $lookup. + */ +(function() { + "use strict"; + + let testDB = db.getSiblingDB("lookup_absorb_match"); + testDB.dropDatabase(); + + let locations = testDB.getCollection("locations"); + assert.writeOK(locations.insert({_id: "doghouse", coordinates: [25.0, 60.0]})); + assert.writeOK(locations.insert({_id: "bullpen", coordinates: [-25.0, -60.0]})); + + let animals = testDB.getCollection("animals"); + assert.writeOK(animals.insert({_id: "dog", locationId: "doghouse"})); + assert.writeOK(animals.insert({_id: "bull", locationId: "bullpen"})); + + // Test that a $match with $geoWithin works properly when performed directly on an absorbed + // lookup field. + let result = testDB.animals + .aggregate([ + { + $lookup: { + from: "locations", + localField: "locationId", + foreignField: "_id", + as: "location" + } + }, + {$unwind: "$location"}, + { + $match: { + "location.coordinates": { + $geoWithin: { + $geometry: { + type: "MultiPolygon", + coordinates: [[[ + [20.0, 70.0], + [30.0, 70.0], + [30.0, 50.0], + [20.0, 50.0], + [20.0, 70.0] + ]]] + } + } + } + } + } + ]) + .toArray(); + let expected = [{ + _id: "dog", + locationId: "doghouse", + location: {_id: "doghouse", coordinates: [25.0, 60.0]} + }]; + assert.eq(result, expected); + + // Test that a $match with $geoIntersects works as expected when absorbed by a $lookup. + result = testDB.animals + .aggregate([ + { + $lookup: { + from: "locations", + localField: "locationId", + foreignField: "_id", + as: "location" + } + }, + {$unwind: "$location"}, + { + $match: { + "location.coordinates": { + $geoIntersects: { + $geometry: { + type: "MultiPolygon", + coordinates: [[[ + [-20.0, -70.0], + [-30.0, -70.0], + [-30.0, -50.0], + [-20.0, -50.0], + [-20.0, -70.0] + ]]] + } + } + } + } + } + ]) + .toArray(); + expected = [{ + _id: "bull", + locationId: "bullpen", + location: {_id: "bullpen", coordinates: [-25.0, -60.0]} + }]; + assert.eq(result, expected); +}()); diff --git a/src/mongo/db/matcher/expression_geo.cpp b/src/mongo/db/matcher/expression_geo.cpp index f229eec42c2..e73efdca1db 100644 --- a/src/mongo/db/matcher/expression_geo.cpp +++ b/src/mongo/db/matcher/expression_geo.cpp @@ -364,7 +364,11 @@ bool GeoMatchExpression::matchesSingleElement(const BSONElement& e) const { void GeoMatchExpression::debugString(StringBuilder& debug, int level) const { _debugAddSpace(debug, level); - debug << "GEO raw = " << _rawObj.toString(); + + BSONObjBuilder builder; + serialize(&builder); + debug << "GEO raw = " << builder.obj().toString(); + MatchExpression::TagData* td = getTag(); if (NULL != td) { debug << " "; @@ -374,7 +378,9 @@ void GeoMatchExpression::debugString(StringBuilder& debug, int level) const { } void GeoMatchExpression::serialize(BSONObjBuilder* out) const { - out->appendElements(_rawObj); + BSONObjBuilder subobj(out->subobjStart(path())); + subobj.appendElements(_rawObj); + subobj.doneFast(); } bool GeoMatchExpression::equivalent(const MatchExpression* other) const { @@ -431,7 +437,9 @@ void GeoNearMatchExpression::debugString(StringBuilder& debug, int level) const } void GeoNearMatchExpression::serialize(BSONObjBuilder* out) const { - out->appendElements(_rawObj); + BSONObjBuilder subobj(out->subobjStart(path())); + subobj.appendElements(_rawObj); + subobj.doneFast(); } bool GeoNearMatchExpression::equivalent(const MatchExpression* other) const { diff --git a/src/mongo/db/matcher/expression_geo.h b/src/mongo/db/matcher/expression_geo.h index 856b31045f8..60ae66f3cb6 100644 --- a/src/mongo/db/matcher/expression_geo.h +++ b/src/mongo/db/matcher/expression_geo.h @@ -108,12 +108,11 @@ public: const GeoExpression& getGeoExpression() const { return *_query; } - const BSONObj getRawObj() const { - return _rawObj; - } private: + // The original geo specification provided by the user. BSONObj _rawObj; + // Share ownership of our query with all of our clones std::shared_ptr _query; bool _canSkipValidation; @@ -183,12 +182,11 @@ public: const GeoNearExpression& getData() const { return *_query; } - const BSONObj getRawObj() const { - return _rawObj; - } private: + // The original geo specification provided by the user. BSONObj _rawObj; + // Share ownership of our query with all of our clones std::shared_ptr _query; }; diff --git a/src/mongo/db/matcher/expression_parser_geo.cpp b/src/mongo/db/matcher/expression_parser_geo.cpp index d706ef65193..433af63ed3d 100644 --- a/src/mongo/db/matcher/expression_parser_geo.cpp +++ b/src/mongo/db/matcher/expression_parser_geo.cpp @@ -53,12 +53,7 @@ StatusWithMatchExpression expressionParserGeoCallbackReal(const char* name, unique_ptr e = make_unique(); - // Until the index layer accepts non-BSON predicates, or special indices are moved into - // stages, we have to clean up the raw object so it can be passed down to the index - // layer. - BSONObjBuilder bob; - bob.append(name, section); - Status s = e->init(name, gq.release(), bob.obj()); + Status s = e->init(name, gq.release(), section); if (!s.isOK()) return StatusWithMatchExpression(s); return {std::move(e)}; @@ -70,12 +65,7 @@ StatusWithMatchExpression expressionParserGeoCallbackReal(const char* name, return StatusWithMatchExpression(s); } unique_ptr e = make_unique(); - // Until the index layer accepts non-BSON predicates, or special indices are moved into - // stages, we have to clean up the raw object so it can be passed down to the index - // layer. - BSONObjBuilder bob; - bob.append(name, section); - s = e->init(name, nq.release(), bob.obj()); + s = e->init(name, nq.release(), section); if (!s.isOK()) return StatusWithMatchExpression(s); return {std::move(e)}; -- cgit v1.2.1