[SERVER-55815] Renaming $set or $addFields incorrectly reports modified paths for optimizations Created: 06/Apr/21  Updated: 06/Dec/22

Status: Backlog
Project: Core Server
Component/s: Query Planning
Affects Version/s: None
Fix Version/s: None

Type: Bug Priority: Major - P3
Reporter: Charlie Swanson Assignee: Backlog - Query Optimization
Resolution: Unresolved Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Assigned Teams:
Query Optimization
Operating System: ALL
Sprint: Query Optimization 2021-05-17, Query Optimization 2021-05-31, Query Optimization 2021-06-14, Query Optimization 2021-06-28, Query Optimization 2021-07-12, Query Optimization 2021-07-26, QO 2021-09-06, Query Optimization 2021-08-09, QO 2021-09-20, QO 2021-10-04, QO 2021-08-23
Participants:

 Description   

Upon inspecting a unit test we recently discovered that the server's logic for analyzing modified and renamed paths seems to mistakenly mark a rename from "x" to "y" like {$set: {y: "$x"}} as only renaming, whereas it should also mark "y" as modified since the old value is getting overridden.



 Comments   
Comment by James Wahlin [ 01/Sep/21 ]

I took a stab at this today, and it appears that marking the rename target as modified interferes with pipeline optimizations that use the rename chain to move stages before the rename. An example being the pipeline: 

[{$addFields: {a: '$b'}}, \{$match: {a: {$eq: 1}}}]
 

 which can be optimized to:

[{$match: {b: {$eq: 1}}}, \{$addFields: {a: '$b'}}]

If we were to proceed with adding the rename target to modified paths, I suspect we would also need to add logic to recognize that a modified path is actually just a rename when optimizing.

charlie.swanson let's discuss whether it makes sense to proceed on this.

For the record my working patch based on commit 23f9d2a53917d63fc3d3b8c8646f40f2bc4caa2f is the following:

diff --git a/src/mongo/db/exec/inclusion_projection_executor_test.cpp b/src/mongo/db/exec/inclusion_projection_executor_test.cpp
index a2d6932a87..582df1c610 100644
--- a/src/mongo/db/exec/inclusion_projection_executor_test.cpp
+++ b/src/mongo/db/exec/inclusion_projection_executor_test.cpp
@@ -405,7 +405,7 @@ TEST_F(InclusionProjectionExecutionTestWithFallBackToDefault,
        ShouldReportThatAllExceptIncludedFieldsAreModifiedWithIdExclusion) {
     auto inclusion = makeInclusionProjectionWithDefaultPolicies(
         BSON("_id" << false << "a" << wrapInLiteral("computedVal") << "b.c"
-                   << wrapInLiteral("computedVal") << "d" << true << "e.f" << true));
+                   << wrapInLiteral("computedVal") << "d" << true << "e.f" << true << "f" << "$f"));
 
     auto modifiedPaths = inclusion->getModifiedPaths();
     ASSERT(modifiedPaths.type == DocumentSource::GetModPathsReturn::Type::kAllExcept);
@@ -415,6 +415,7 @@ TEST_F(InclusionProjectionExecutionTestWithFallBackToDefault,
     // Computed paths are modified.
     ASSERT_EQ(modifiedPaths.paths.count("a"), 0UL);
     ASSERT_EQ(modifiedPaths.paths.count("b.c"), 0UL);
+    ASSERT_EQ(modifiedPaths.paths.count("f"), 1UL);
     // _id is explicitly excluded.
     ASSERT_EQ(modifiedPaths.paths.count("_id"), 0UL);
 
diff --git a/src/mongo/db/exec/projection_node.cpp b/src/mongo/db/exec/projection_node.cpp
index 1ef569ecd7..3919f45188 100644
--- a/src/mongo/db/exec/projection_node.cpp
+++ b/src/mongo/db/exec/projection_node.cpp
@@ -250,6 +250,7 @@ void ProjectionNode::reportComputedPaths(std::set<std::string>* computedPaths,
 
         for (auto&& rename : exprComputedPaths.renames) {
             (*renamedPaths)[rename.first] = rename.second;
+            computedPaths->insert(rename.first);
         }
     }
     for (auto&& childPair : _children) {
diff --git a/src/mongo/db/pipeline/document_source_add_fields_test.cpp b/src/mongo/db/pipeline/document_source_add_fields_test.cpp
index 9d3227f841..079f3715e2 100644
--- a/src/mongo/db/pipeline/document_source_add_fields_test.cpp
+++ b/src/mongo/db/pipeline/document_source_add_fields_test.cpp
@@ -271,7 +271,7 @@ TEST_F(AddFieldsTest, TestModifiedPaths) {
     auto modifiedPaths = addFields->getModifiedPaths();
 
     ASSERT(modifiedPaths.type == DocumentSource::GetModPathsReturn::Type::kFiniteSet);
-    ASSERT_EQUALS(1U, modifiedPaths.paths.size());
+    ASSERT_EQUALS(2U, modifiedPaths.paths.size());
     ASSERT_EQUALS(1U, modifiedPaths.renames.size());
 }
 }  // namespace

Generated at Thu Feb 08 05:37:33 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.