From 35a4ab98a2378bc5699abe9cdc1e554da7ec79d7 Mon Sep 17 00:00:00 2001 From: Hana Pearlman Date: Wed, 13 Sep 2023 16:06:49 +0000 Subject: [PATCH] SERVER-80976: Fall back to classic on dotted comparison to null --- jstests/core/cover_null_queries.js | 4 +- jstests/core/query/array/arrayfind5.js | 4 -- jstests/noPassthrough/cqf_fallback.js | 38 ++++++++++++++++--- .../randomized_mixed_type_bug.js | 8 +--- src/mongo/db/query/cqf_command_utils.cpp | 28 +++++++++++--- 5 files changed, 58 insertions(+), 24 deletions(-) diff --git a/jstests/core/cover_null_queries.js b/jstests/core/cover_null_queries.js index 3cf05831044..1b32a20727e 100644 --- a/jstests/core/cover_null_queries.js +++ b/jstests/core/cover_null_queries.js @@ -6,8 +6,6 @@ * requires_fcv_62, * # This test could produce unexpected explain output if additional indexes are created. * assumes_no_implicit_index_creation, - * # TODO SERVER-67506: Dotted path equality to null matches non-object array elements in CQF. - * cqf_incompatible, * ] */ import {arrayEq} from "jstests/aggregation/extras/utils.js"; @@ -1019,4 +1017,4 @@ validateGroupCountAggCmdOutputAndPlan({ filter: {"a.b": {$in: [null, []]}}, expectedCount: 10, expectedStages: {"IXSCAN": 1, "FETCH": 1}, -}); \ No newline at end of file +}); diff --git a/jstests/core/query/array/arrayfind5.js b/jstests/core/query/array/arrayfind5.js index 6238284f89a..b530a0d8e4e 100644 --- a/jstests/core/query/array/arrayfind5.js +++ b/jstests/core/query/array/arrayfind5.js @@ -1,8 +1,4 @@ // Test indexed elemmatch of missing field. -// @tags: [ -// # TODO SERVER-67506: Dotted path equality to null matches non-object array elements in CQF. -// cqf_incompatible, -// ] let t = db.jstests_arrayfind5; t.drop(); diff --git a/jstests/noPassthrough/cqf_fallback.js b/jstests/noPassthrough/cqf_fallback.js index 6eb1b4bf362..952416e6b63 100644 --- a/jstests/noPassthrough/cqf_fallback.js +++ b/jstests/noPassthrough/cqf_fallback.js @@ -111,6 +111,27 @@ assertNotSupportedByBonsai({find: coll.getName(), filter: {$alwaysFalse: 1}}, tr assertNotSupportedByBonsai( {aggregate: coll.getName(), pipeline: [{$match: {$alwaysFalse: 1}}], cursor: {}}, true); +// Test $match against null. When field paths are dotted, these have testOnly support. When the +// field paths are not dotted, these are fully supported. +assertSupportedByBonsaiFully({find: coll.getName(), filter: {'a': {$eq: null}}}); +assertSupportedByBonsaiFully({find: coll.getName(), filter: {'a': {$lte: null}}}); +assertSupportedByBonsaiFully({find: coll.getName(), filter: {'a': {$gt: null}}}); +assertSupportedByBonsaiFully( + {aggregate: coll.getName(), pipeline: [{$match: {a: {$eq: null}}}], cursor: {}}); +assertSupportedByBonsaiFully({find: coll.getName(), filter: {a: {$in: [1, 2, null, 3]}}}) +assertSupportedByBonsaiFully({find: coll.getName(), filter: {a: {$elemMatch: {b: null}}}}); +assertSupportedByBonsaiFully({find: coll.getName(), filter: {'a.c': {$elemMatch: {b: null}}}}); + +assertNotSupportedByBonsai({find: coll.getName(), filter: {'a.b': {$eq: null}}}, true); +assertNotSupportedByBonsai({find: coll.getName(), filter: {'a.b': {$lte: null}}}, true); +assertNotSupportedByBonsai({find: coll.getName(), filter: {'a.b': {$gt: null}}}, true); +assertNotSupportedByBonsai( + {aggregate: coll.getName(), pipeline: [{$match: {'a.b.c': {$eq: null}}}], cursor: {}}, true); +assertNotSupportedByBonsai({find: coll.getName(), filter: {'a.b': {$in: [1, 2, null, 3]}}}, true); +assertNotSupportedByBonsai({find: coll.getName(), filter: {a: {$elemMatch: {'b.c': null}}}}, true); +assertNotSupportedByBonsai({find: coll.getName(), filter: {'a.c': {$elemMatch: {'b.c': null}}}}, + true); + // Test $match on _id; these have only experimental support. assertSupportedByBonsaiExperimentally({find: coll.getName(), filter: {_id: 1}}); assertSupportedByBonsaiExperimentally( @@ -629,23 +650,30 @@ MongoRunner.stopMongod(conn); // Show that we can't start a mongod with the framework control set to tryBonsaiExperimental when // test commands are off. TestData.enableTestCommands = false; +TestData.setParameters.internalQueryFrameworkControl = "tryBonsaiExperimental"; try { - conn = MongoRunner.runMongod( - {setParameter: {internalQueryFrameworkControl: "tryBonsaiExperimental"}}); + conn = MongoRunner.runMongod(); MongoRunner.stopMongod(conn); assert(false, "MongoD was able to start up when it should have failed"); -} catch (_) { +} catch (e) { // This is expected. + assert.eq(e.returnCode, + ErrorCodes.BadValue, + "Expected a BadValue error, but encountered: " + e.message); } // Show that we can't start a mongod with the framework control set to tryBonsai // when the feature flag is off. TestData.setParameters.featureFlagCommonQueryFramework = false; TestData.enableTestCommands = true; +TestData.setParameters.internalQueryFrameworkControl = "tryBonsai"; try { - conn = MongoRunner.runMongod({setParameter: {internalQueryFrameworkControl: "tryBonsai"}}); + conn = MongoRunner.runMongod(); MongoRunner.stopMongod(conn); assert(false, "MongoD was able to start up when it should have failed"); -} catch (_) { +} catch (e) { // This is expected. + assert.eq(e.returnCode, + ErrorCodes.BadValue, + "Expected a BadValue error, but encountered: " + e.message); } diff --git a/jstests/noPassthroughWithMongod/randomized_mixed_type_bug.js b/jstests/noPassthroughWithMongod/randomized_mixed_type_bug.js index c62de4e5019..ce3ccef2af2 100644 --- a/jstests/noPassthroughWithMongod/randomized_mixed_type_bug.js +++ b/jstests/noPassthroughWithMongod/randomized_mixed_type_bug.js @@ -2,15 +2,9 @@ * Tests that randomly generated documents can be queried from timeseries collections in the same * manner as a tradional collection. */ -import {checkCascadesOptimizerEnabled} from "jstests/libs/optimizer_utils.js"; import {fc} from "jstests/third_party/fast_check/fc-3.1.0.js"; -// TODO SERVER-67506: Re-enable this test when a decision is made about how Bonsai will handle -// comparison to null. Other semantic difference tickets are also relevant here. -let scalars = [fc.string(), fc.double(), fc.boolean(), fc.date()]; -if (!checkCascadesOptimizerEnabled(db)) { - scalars.push(fc.constant(null)); -} +const scalars = [fc.string(), fc.double(), fc.boolean(), fc.date(), fc.constant(null)]; const pathComponents = fc.constant("a", "b"); // Define our grammar for documents. diff --git a/src/mongo/db/query/cqf_command_utils.cpp b/src/mongo/db/query/cqf_command_utils.cpp index 195c8245a00..093adfa09c9 100644 --- a/src/mongo/db/query/cqf_command_utils.cpp +++ b/src/mongo/db/query/cqf_command_utils.cpp @@ -145,10 +145,10 @@ public: _queryHasNaturalHint(queryHasNaturalHint) {} void visit(const LTEMatchExpression* expr) override { - assertSupportedPathExpression(expr); + assertSupportedComparisonMatchExpression(expr); } void visit(const LTMatchExpression* expr) override { - assertSupportedPathExpression(expr); + assertSupportedComparisonMatchExpression(expr); } void visit(const ElemMatchObjectMatchExpression* expr) override { assertSupportedPathExpression(expr); @@ -157,17 +157,25 @@ public: assertSupportedPathExpression(expr); } void visit(const EqualityMatchExpression* expr) override { - assertSupportedPathExpression(expr); + assertSupportedComparisonMatchExpression(expr); } void visit(const GTEMatchExpression* expr) override { - assertSupportedPathExpression(expr); + assertSupportedComparisonMatchExpression(expr); } void visit(const GTMatchExpression* expr) override { - assertSupportedPathExpression(expr); + assertSupportedComparisonMatchExpression(expr); } void visit(const InMatchExpression* expr) override { assertSupportedPathExpression(expr); + // Dotted path equality to null is not supported. + const auto fieldRef = expr->fieldRef(); + if (fieldRef && fieldRef->numParts() > 1) { + _eligible &= std::none_of(expr->getEqualities().begin(), + expr->getEqualities().end(), + [](auto&& elt) { return elt.isNull(); }); + } + // $in over a regex predicate is not supported. if (!expr->getRegexes().empty()) { _eligible = false; @@ -370,6 +378,16 @@ private: _eligible = false; } + void assertSupportedComparisonMatchExpression(const ComparisonMatchExpression* expr) { + assertSupportedPathExpression(expr); + + // Dotted path equality to null is not supported. + const auto fieldRef = expr->fieldRef(); + if (fieldRef && fieldRef->numParts() > 1 && expr->getData().isNull()) { + _eligible = false; + } + } + void assertSupportedPathExpression(const PathMatchExpression* expr) { const auto fieldRef = FieldRef(expr->path()); if (fieldRef.hasNumericPathComponents())