Expand correlated subquery testing for $lookup stage

[danielfrankcom]

This change adds additional testing for correlated $lookup, focusing on let variable value forms and the scoping of $$ROOT/$$CURRENT within the sub-pipeline.

This is related to #673, in particular this comment thread.

Changes based on @PatersonProjects'.

[documentdb-triage-tool]

🤖 Auto-triaged by documentdb-triage-tool.

Applied: compatibility test, enhancement
Project fields suggested: Component test-coverage · Priority P3 · Effort M · Status Needs Review
Confidence: 0.82 (mixed)

Reasoning

component from path globs (test-coverage); effort from diff stats (334+0 LOC, 2 files); LLM: Adds additional test cases for correlated $lookup scenarios (let variables, $$ROOT/$$CURRENT scoping) with no production code changes.

If a label is wrong, remove it manually and ping @patty-chow so the rules can be tuned. The bot will not re-label items that already have component labels.

[yshanhu]

With the purpose of checking let passing through for all BSON types:

Can we please:
1.add more types:
e.g.

  # Add these 9 to the existing 7 types:
  "v_int64": Int64(100),
  "v_decimal128": Decimal128("123.45"),
  "v_objectid": ObjectId("507f1f77bcf86cd799439011"),
  "v_date": datetime(2024, 1, 1, tzinfo=timezone.utc),
  "v_binary": Binary(b"test_data", 0),
  "v_timestamp": Timestamp(1234567890, 1),
  "v_regex": Regex("^test", "i"),
  "v_minkey": MinKey(),
  "v_maxkey": MaxKey(),

2.modify this query shape to also check if type preserved, see example here

db.local.aggregate([
      {
          $lookup: {
              from: "foreign",
              let: {
                  vi: "$v_int",
                  ...
              },
              pipeline: [
                  {
                      $addFields: {
                          // Values
                          ri: "$$vi",
                         ...
                          // Types
                          type_i: { $type: "$$vi" },
                      }
                  }
              ],
              as: "joined"
          }
      }
  ]);

Note:
If we have not already, in the verbose match stage specific test, lets add a test with all BSON type again to test equality matching, not just passing through

[danielfrankcom]

I'll refer to the additional note at the bottom as "3." just for clarity:

1. Done in 3b9a2d5. The let passthrough test now covers all BSON types as variables, including the ones you listed.

2. The type is already enforced by the test framework, just at the assertion layer rather than in the query shape. The comparison runs through _strict_equal, which rejects cross-type numerics. I don't think we need an additional $type field here, as it would just be testing the type server side instead of client side.

3. I don't think this belongs in the $lookup test files. FOLDER_STRUCTURE.md treats $lookup as a container and says to keep sub-feature testing minimal:

   Container features ($expr, $match, $lookup): under the container's directory, only test that sub-features work inside it, one test case per sub-feature, no edge cases. Edge cases belong in each sub-feature's own directory.
   $lookup/ → 1-2 cases per pipeline sub-stage

   Equality matching across BSON types is edge-case coverage of $eq, and it already lives in comparisons/eq/. Re-running the full matrix here would duplicate that, and would also be inconsistent with the way we have handled this for other operators that combine with $eq.

[yshanhu]

Thanks for Point 1 and 2!

For point 3, $match + $expr + $eq behavior is different from $eq, should be tested with all data types.
Is this the directory you refer to? If yes, only one case in find is not sufficient. If elsewhere, please let me know, I will do a quick check

Agree not to add here: test_lookup_correlated_subquery.py, should be added in test_verbose_match.py

[danielfrankcom]

I think there are two different $eq operators in play here, which is probably the source of the confusion.

The $eq used inside $expr is the aggregation expression operator, {$eq: [a, b]}. That one is already covered with a full type matrix in the expression $eq directory.

The directory you linked is the query operator, {field: {$eq: v}}. It only has a smoke test right now, but the coverage for it is up for review in #696. $match + $expr + $eq doesn't use that operator, it uses the expression one.

There are a number of operators which do comparisons like this between BSON types, and we had decided to extract this common behavior to the expression $eq directory to avoid duplicating such tests everywhere. I think the same applies here.

[yshanhu]

Noticing all the let form tests were in let+pipeline syntax for correlated, correct me if anything I miss checking:

Can we add at least one test for:
1/concise correlated? (localField+foreignField + let + pipeline), we can do just one test but with all possible forms of let (constant/expressions/field reference/system variable) in this type of syntax;
2/add one test where let is not provided, since it is optional ->expect to function as uncorrelated.

[danielfrankcom]

Both of these are already covered, just in sibling files that aren't changed in this PR.

The concise correlated form (localField + foreignField + let + pipeline) is tested in test_lookup_concise_correlated_subquery.py, including a case that binds a let variable alongside the equality match and reads it in the sub-pipeline. I don't think we need to test the full form matrix (constant, expression, field reference, system variable) for every syntax, it lives once in test_lookup_correlated_subquery.py and seems like it falls under the repo guidance of not retesting behaviors in different contexts.

The "let is not provided" case is covered in test_lookup_uncorrelated_subquery.py, which has a test that covers "pipeline and no let runs the sub-pipeline independently." That file also shows let: null and let: {} behave the same as omitting it.

[yshanhu]

In testing let as a field reference, can we add a case for nested field reference?

for exmaple:
db.local.insertMany([
      {_id: 1, data: {item: 42}},           // Nested scalar
      {_id: 2, data: {item: [1, 2, 3]}},    // Nested array
      {_id: 3, data: {item: "hello"}},      // Nested scalar (string)
  ]);

in correlated lookup: let: {localItem: "$data.item"}, then use $$localItem in the inner subpipeline

[danielfrankcom]

I think this is effectively covered by sibling files which aren't in this PR.

The different path shapes are covered in test_lookup_field_path_traversal.py by a nested object path (nested.val), a composite array path (arr.val over an array of objects), and an array index path (arr.0) test.

That BSON type compatibility is covered by the all types passthrough case we discussed elsewhere, but combining that with the field path shapes testing would be doing a cross-product of different test axes which I don't think we want to start doing.

Aside from the patterns we've established with other operator tests, the repo docs have some references to this:

Do NOT exhaustively test field path resolution like "$a.b.c" on deeply nested arrays (this belongs to the expression engine tests under expressions/)

DO add one smoke test that [the operator] accepts a field path input (confirms wiring, not field path semantics)