Add correlated lookup tests

[PatersonProjects]

This PR adds additional tests to the lookup operator so as to test stage-specific coverage, $let with various operators, and edge and error cases

Ref: Issue #34

[documentdb-triage-tool]

🤖 Auto-triaged by documentdb-triage-tool.

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

Reasoning

component from path globs (test-coverage); effort from diff stats (4560+59 LOC, 10 files); LLM: Adds new test cases for the lookup operator covering stage-specific scenarios, $let operators, and edge/error cases, expanding test coverage without modifying core functionality.

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]

Thanks for putting these great tests together! Nice work!

As we can see, a more organized folder structure for lookup will be necessary, given the complexity, to make sure 1.test coverage 2.no redundancy 3.readability. What do you think about the following folder structure?

 tests/core/operator/stages/lookup/
  │
  ├── test_lookup_standard.py                    # No changes needed here
  ├── test_lookup_uncorrelated_pipeline.py       # No changes needed here
  ├── test_lookup_correlated_basics.py           # let mechanism fundamentals
  │   # 1.All forms for let(constants, expressions, field references, $$ROOT + $$CURRENT system variables)
  │     2.errors cases (let validation:rejects invalid let variable names: dots, $, uppercase start, empty, and non-alphanumeric chars;) 
  |     3.Edge cases (let variable resolves to null/missing)Some system vars not supported to be used in let, should error out
  |     4.result cardinality(less important) + BSON types should be preserved
  │
  ├── concise/                                    # localField + foreignField + pipeline (let optional)
  │   ├── test_concise_equality_matching.py      # localField + foreignField Equality behavior, all BSON types, arrays 
  │   ├── test_concise_pipeline_execution.py     # each test with one stage:$match,$group,$project,$bucket, $bucketAuto, $addfields,$set,$replaceroot,$geoNear, $sortByCount, $setWindowFields, $graphlookup, their fields accessing let varaibles --check if let is recognized and resolved as expected
  │   ├── test_concise_let_variables.py          # let as optional, forms, interaction 
  │   ├── test_concise_degradation.py            # Pending testing, lets make sure this is expected 2-3 tests is good
  │   ├── test_concise_stage_combinations.py     # Multi-stage patterns, combination of the stages mentioned above, mixed with stages that do not access let variables
  │   └── test_concise_errors.py                 # Invalid syntax
  │
  ├── verbose/                                    # let + pipeline only
  │   ├── test_verbose_match.py                  # $expr ($eq,$gt,$lt,$gte,$lte comparison operators),$add,$or,$not Logical Operators, array and type operators($in, $type), check numeric equivelence:e.g.int32=1, int64=1, double=1.0, Decimal128("1") all match, false ≠ 0, true ≠ 1,"" ≠ null, null≠missing, Regular $match without $expr
  │   ├── test_verbose_project.py                # Basic wiring(Simple variable projection, multiple variables, variables with existing fields); Expression category(Arithmetic Expressions,String Expressions,Date Expressions, Conditional Expressions, array expression,Type/Comparison Expressions); Nested/Complex Expressions, edge case, error case
  │   ├── test_verbose_addfields.py              # $addFields wiring 
  │   ├── test_verbose_group.py                  # accumulators, _id access let var
  │   ├── test_verbose_bucketAuto.py        # expression field access let var 
  │   ├── test_verbose_$geoNear.py                 # expression field access let var
  │   ├── test_verbose_bucket.py                 # expression field access let var
  │   ├── test_verbose_redact.py                 # expression field access let var
  │   ├── test_verbose_replaceroot.py            # expression field access let var
  │   ├── test_verbose_$setWindowFields.py            # expression field access let var
  │   ├── test_verbose_$sortByCount.py            # expression field access let var
  │   ├── test_verbose_$graphlookup.py            # expression field access let var
  │   ├── test_verbose_nested_lookup.py          # Variable scoping, shadowing (combination of standard lookup, uncorrelated and correlated lookup)
  │   ├── test_verbose_stage_combinations.py     # Multi-stage interactions, some stages, $sort, $unwind, $skip, $limit etc do not use let vars, could be in the inner pipeline 
  │   └── test_verbose_unsupported_stages.py     # Error tests: $merge, $out in the inner pipeline
  │
  ├── collation/                                  # 1.command level/collection level/index level collation
  │   ├── test_collation_concise.py              # Concise + collation 
  │   ├── test_collation_verbose.py              # Verbose + collation 
  │   └── test_collation_nested.py               # Propagation 
  │
  └── utils/
      └── lookup_common.py                        # Shared test utilities

[yshanhu]

I'll recommend to break this correlated lookup task into 4 manageable PRs, feel free to communicate with team if this requires a bit more time.

1/let basics (the focus here is not complex stage, use let + pipeline(one imple $match with $expr) will be sufficient);

2.verbose (let + pipeline: the focus here is stage with let wiring. The tests will be more stage behavior focused, we want to test on 1.if let is recognized 2.if let is resolved correctly. See test_verbose_project.py for example)

3.concise (local+foreign+let+pipeline: the focus here is stage with let wiring and some edge cases(array matching, null/missing etc))

4.collation in lookup: we care about
a. Precedence rules: Command-level vs collection-level collation
b. Equality match interaction: Collation affects localField/foreignField matching
c. Subpipeline propagation: Collation inheritance into nested pipelines
d. Cross-collection behavior: Local and foreign collections may have different collations

Feel free to let me know if anything could be changed or needs clarification. Thank you!

[PatersonProjects]

I agree the current state is unwieldy, and like your suggested folder structure above. I will refactor these changes into one PR for each folder.

[PatersonProjects]

On further reflection, I am wondering if we should keep the collation tests to test_stages_lookup, and simply expand that file to cover concise and verbose. That change can still be handled in a separate PR but it will keep the crossover in the parent folder.

[yshanhu]

Thanks for the suggestion. Yes, adding tests to test_stages_lookup is ok, but lets add a comment to indicate that collation in lookup stage only tested in command level is not sufficient since it handles two collections and potentially a subpipeline.
e.g.

"""Tests for collation interaction with $lookup stage.
  
Note: While this file is under collation/command_level/, testing only command-level 
collation is insufficient for $lookup because it uniquely handles 1) TWO collections 
(each with potentially different collations), 2) a subpipeline (requiring collation 
propagation to all stages), and 3) nested $lookups (requiring multi-level collation 
propagation), necessitating comprehensive testing beyond just command-level collation.

"""

If we expand the tests in the test_stages_lookup file, type 1), 2) and 3) still should be tested in verbose and concise correlated lookup. Does this make sense?

[danielfrankcom]

Just catching up on this as @PatersonProjects is switching to a different project, so I'm going to finish this task up.

I dug into how collation is handled elsewhere and I think we can keep all of this in the collation directory rather than splitting it into the lookup tests.

FOLDER_STRUCTURE.md is explicit that collation is cross-cutting:

Cross-cutting features (rbac, transactions, collation, namespace rules) → own top-level folders, NOT under feature-specific folders

The concern about command-level testing being insufficient is valid, but $unionWith already sets a precedent that pulls in a foreign collection within a sub-pipeline for collation testing. test_stages_unionwith.py covers both propagation into the sub-pipeline and command-level collation overriding the foreign collection's default.

I think we can do something similar for $lookup and avoid spreading collation testing around the repo. We can expand test_stages_lookup.py for propagation, foreign override, and nested lookups, and put precedence testing in resolution/test_resolution_precedence.py where the inheritance tests already live. That also leaves the verbose and concise files free of collation, since it isn't a $lookup parameter.

[yshanhu]

Makes sense!
test_stages_lookup.py ->add pipeline propagation, foreign override, and nested lookup (don't forget to add 1.different strength levels tests:not just strength 1; 2.tests in both concise/verbose syntax; 3.some edge cases(missing/null/empty pipeline etc)
test_resolution_precedence.py ->test command/collection/index level precedence in both syntax of correlated lookup

[danielfrankcom]

I split the collation work out into #697. I added a few tests based on your suggestions here, but there are some things I'd avoid testing in this context:

Strength levels: Kept to one representative strength. Strength testing already lives under collation/options/, and it's a property of collation itself which is tested in that context. Cross-cutting behaviors like this are typically tested only in the context of their own directory, as testing them everywhere would create a massive matrix of redundant tests.

Precedence in both syntaxes: I think there are really two separate axes here, and each is already covered. Whether a given collation actually reaches each form (concise/verbose) is the propagation testing. Which collation wins is its own set of precedence cases. Testing precedence per syntax just combines those two, so it doesn't exercise anything the per-axis tests don't already cover. Given there are many such axes we have tried to avoid doing matrix testing like this in the project as it will blow up in complexity fast.

Index level: Added a case showing a collated foreign-field index doesn't change the join result, but similar to above this seems like its own axis which doesn't warrant testing across both syntactic sugar forms.

Missing/null/empty pipeline: These concerns are covered in the general $lookup pipeline validation rather than collation.

[danielfrankcom]

Also published #698 for the "basics" coverage. Most was already handled, though I added a few cases. Will work on the concise/verbose subdirectories from the suggested structure next.

[danielfrankcom]

#699 provides the verbose syntax coverage split.