Add $max accumulator tests

[alinaliBQ]

This change adds tests for the $max accumulator operator.

Add accumulator operator tests for $max. Tests database $max behavior, output collection, syntax, and expected errors.
Integration tests are in documentdb_tests/compatibility/tests/core/operator/accumulators/test_accumulators_max_integration.py

• Add compatibility test for accumulator operators (11 total) #28

[documentdb-triage-tool]

🤖 Auto-triaged by documentdb-triage-tool.

Applied: compatibility test, enhancement
Project fields suggested: Component test-coverage · Priority P2 · Effort XL · Status In Progress
Confidence: 0.92 (mixed)

Reasoning

component from path globs (test-coverage, test-framework); effort from diff stats (3686+2 LOC, 11 files); LLM: Adds new integration test coverage for the $max accumulator operator under the compatibility tests path, a meaningful functional addition with no schema or cross-component 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.

[eerxuan]

The test file names of all accumulator tests should align. Change other tests to use the same prefix, test_accumulator_*.py

[eerxuan]

Suggestion for the test layout going forward: stage-level facts (e.g., the $bucketAuto Code/MaxKey serialization quirk, error-code differences between $divide in $group vs $bucketAuto) are properties of the stage, not the accumulator. Today they're being tested redundantly under each accumulators//test_*_stage_divergence.py. By the time the catalog is complete (~10 accumulators), we'll have ~10 copies of the same stage-level assertions.
Consider moving stage-level coverage into stages/{group,bucket,bucketAuto}/ and using a representative accumulator group as the test driver.

[alinaliBQ]

Suggestion for the test layout going forward: stage-level facts (e.g., the $bucketAuto Code/MaxKey serialization quirk, error-code differences between $divide in $group vs $bucketAuto) are properties of the stage, not the accumulator. Today they're being tested redundantly under each accumulators//test_*_stage_divergence.py. By the time the catalog is complete (~10 accumulators), we'll have ~10 copies of the same stage-level assertions. Consider moving stage-level coverage into stages/{group,bucket,bucketAuto}/ and using a representative accumulator group as the test driver.

I will remove all stages tests in accumulators/. Currently the accumulator tests are covered with:

• $bucket - test_bucket_accumulator_operators.py
• $group - test_group_accumulators.py and a couple of other files in group

Edit: I think since stage tests are covering accumulators, I can remove all stage related tests in accumulators folder.

[alinaliBQ]

Renamed files to test_accumulator_*.py, also addressed comments for the $sum tests

[alinaliBQ]

Deleted this file since they are stage tests.
Edit: this file was renamed to test_accumulator_sum_errors.py, not deleted

[eerxuan]

arity errors should not be removed.

[alinaliBQ]

got it, restored arity tests for $max and $sum. $sum arity tests are restored to test_accumulator_sum_errors.py.

[alinaliBQ]

Removed Code tests from sum too

[eerxuan]

Please reference other operator tests and TEST_COVERAGE.md. This PR is missing:

• Argument validation tests.
  • Arity test, $max should only take one argument. (4. Argument Handling)
  • Test every bson type in the argument, should return error for invalid types. (1. Data Type Coverage in TEST_COVERAGE.md)
• One test with every type of expressions. Expression Types: (3. Expression Type Coverage)

With expression operator: {$max: {$add: [1, 2]}}. (11. Expression Operator in Pipeline Contexts)
With array expression input: {$max: [["$x", "$y"]]} — array containing field references
With object expression input: {$max: {a: "$x"}} — object with field reference values
With composite array input: doc {a: [{b: 1}, {b: 2}]}, expression {$max: "$a.b"} — field path resolving to array from array-of-objects

Please raise a separate PR to add rules for accumulator so we don't have similar confusions in future. Please suggest changes on the rules as you gain more understanding on any feature.

### N. Accumulator Coverage

**Rule**: Each accumulator must be tested for its expression error propagation, empty-group result, order dependence, and sibling-accumulator interactions. Tests live under
  `tests/core/operator/accumulators/$accumulator/`. Sibling-accumulator interactions live in `tests/core/operator/accumulators/test_accumulators_$op_integration.py`.

  The expression-form of dual-form operators (`$max`, `$min`, `$sum`, `$avg`, `$first`, `$last`, etc.) is tested separately under `tests/core/operator/expressions/accumulator/$op/` and is out of scope for the
  accumulator-form file.

  **Expression Error Propagation**:
  Errors raised during sub-expression evaluation propagate through the accumulator without being caught:
  - `{$op: {$divide: [1, "$v"]}}` → `CONVERSION_FAILURE_ERROR` (field violation)
  - `{$op: {$divide: ["$v", 0]}}` → `DIVIDE_BY_ZERO_V2_ERROR` (literal violation)

  **Empty-Group Behavior**:
  Each accumulator defines a result for an empty group (zero matching documents). Verify the documented value for `{$group: {_id: null, r: {$op: ...}}}` against an empty collection. Result varies per accumulator
  (e.g. 0, null, [], {}) — refer to the accumulator's spec.

  **Order Dependence**:
  Some accumulators are order-dependent (their result depends on input order); others are order-independent. Verify per accumulator which category it belongs to per its spec, and:

  - For order-dependent accumulators, tests asserting a specific result must include a preceding `$sort`. Tests without `$sort` are flaky. (e.g. $first)
  - For order-independent accumulators, the result must be the same regardless of input order. Verify this by running the same input twice with different `$sort` directions and asserting identical results.

  **Tested in Other Folders** (in scope, but add under a different folder):
  - **Host-stage compatibility** — when adding a new accumulator, add one smoke case for each host stage that supports it (`$group`, `$bucket`, `$bucketAuto`, `$setWindowFields`) under that stage's folder
  (`stages/$stage/`), per the container-features rule (one test per sub-feature, no edge cases). Edge cases that are genuinely accumulator-specific stay in `accumulators/$op/`.
  - **Stage-level error codes** (e.g. `$bucketAuto` wrapping `DIVIDE_BY_ZERO_V2_ERROR` as `BadValue`) — under `stages/bucketAuto/`, parameterized over a representative accumulator.

 ** Scope that Covered by Other rules**:
  - **Numeric equivalence in grouping** (used by `$addToSet`, `$setUnion`) — covered by §9 Numeric Equivalence in Grouping/Comparison.
  - **Output type validation** — covered by §1 Data Type Coverage.

 **Out of Scope**:
  - **BSON comparison ordering** (used by `$max`/`$min`/`$top`/`$bottom`) — `bson_types/test_bson_types_ordering.py`; per-accumulator coverage limited to a small wiring sample.

[eerxuan]

We consider the Bson order a high level spec or contract. We expect every feature respect it, so we will test under bson-types/ folder. That's why we didn't have similar test for query comparison operators.

[alinaliBQ]

yup, removed BSON order tests

[eerxuan]

arity errors should not be removed.

[alinaliBQ]

@eerxuan Ty for feedback. I have:

• added back arity tests
• added BSON argument tests. Accumulators $max and $sum accept all BSON types, so no errors will be thrown
• added expression tests and expression error propagation tests
• added order dependence tests

I have raised PR (#245) for adding the accumulator coverage section to TEST_COVERAGE.md. Thank you for posting the section, it helped to clarify items in scope. I will go through all open accumulator PRs (such as $min #219) and apply the same comments.

[eerxuan]

Remove Code, please add a rule to TEST_COVERAGE.md for deprecated features that we don't want to test. Add Code data type and mapReduce, reIndex, function, $accumulator, $where, currentOp command, filemd5, collStats command.

[alinaliBQ]

My bad, not sure why Code is still there, will work on the comments tomorrow and remove it