Skip to content

Add $setUnion accumulator tests#254

Open
alinaliBQ wants to merge 13 commits into
documentdb:mainfrom
alinaliBQ:setUnion
Open

Add $setUnion accumulator tests#254
alinaliBQ wants to merge 13 commits into
documentdb:mainfrom
alinaliBQ:setUnion

Conversation

@alinaliBQ
Copy link
Copy Markdown
Contributor

This change adds tests for the $setUnion accumulator operator.

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

@alinaliBQ alinaliBQ requested a review from a team as a code owner May 29, 2026 22:31
@alinaliBQ alinaliBQ marked this pull request as draft May 29, 2026 22:31
Comment on lines +280 to +309
AccumulatorTestCase(
"error_prop_divide_by_zero_field_path",
docs=[{"_id": 0, "v": 0}],
pipeline=[
{"$sort": {"_id": 1}},
{
"$group": {
"_id": None,
"result": {"$setUnion": {"$let": {"vars": {}, "in": [{"$divide": [1, "$v"]}]}}},
}
},
],
error_code=DIVIDE_BY_ZERO_V2_ERROR,
msg="$setUnion should propagate $divide by zero when divisor comes from field path",
),
AccumulatorTestCase(
"error_prop_divide_by_zero_later_doc",
docs=[{"_id": 0, "v": 1}, {"_id": 1, "v": 0}],
pipeline=[
{"$sort": {"_id": 1}},
{
"$group": {
"_id": None,
"result": {"$setUnion": {"$let": {"vars": {}, "in": [{"$divide": [1, "$v"]}]}}},
}
},
],
error_code=DIVIDE_BY_ZERO_V2_ERROR,
msg="$setUnion should propagate error even when failing doc is not the first",
),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added 2 divide tests. Also no pytest.approx(math.nan... in this PR

@alinaliBQ alinaliBQ marked this pull request as ready for review May 29, 2026 22:40
@documentdb-triage-tool
Copy link
Copy Markdown

🤖 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.92 (mixed)

Reasoning

component from path globs (test-coverage); effort from diff stats (2304+0 LOC, 9 files); LLM: Adds new integration test coverage for the $setUnion accumulator operator under the compatibility tests path, expanding functional test surface area.

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.

@documentdb-triage-tool documentdb-triage-tool Bot added compatibility test Compatibility test related enhancement New feature or request labels May 29, 2026
msg="$setUnion should work with compound _id grouping key",
),
]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a test which put documents into different groups?

Here most of them have _id:null

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having documents into different groups might be an out of scope as @eerxuan stated here: #257 (comment)
we are only testing the $setUnion accumulator, not broader document grouping

]

# Property [Object Element Deduplication]: objects are compared by structure
# including key order.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test with mixed element types within the same array?

Eg:

 docs=[
     {"v": [1, "a", {"id": 1}, None, [1, 2]]},
     {"v": ["a", 2, {"id": 1}, None]}, ]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added SETUNION_MIXED_ELEMENT_TYPE_TESTS for this

]

# Property [Grouping Key Variations]: $setUnion works with different _id
# grouping expressions.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets also add a test where we are doing $group followed by a $group, with both $setUnion accumulator ops

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added SETUNION_CHAINED_GROUP_TESTS for this

msg="$setUnion should accept simple field path expression",
),
AccumulatorTestCase(
"expr_nested_field_path",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here can we add a test case where nested path is missing in one doc.

like.

docs=[
     {"a": {"b": [1, 2]}},
     {"a": {}},   -----------------------> a exists, b missing
     {"a": {"b": [2, 3]}},
     {"c": 1}, ----------->a missing entirely (combination where a and b are missing and we are doing set union on "$a", $a.b, $a.b.c etc where a is array+ non array case , b is array + non array ......)
 ]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests SETUNION_MISSING_NESTED_PATH_TESTS for this

alinaliBQ added 9 commits June 2, 2026 13:52
Signed-off-by: Alina (Xi) Li <Alina.Li@improving.com>
Signed-off-by: Alina (Xi) Li <Alina.Li@improving.com>
Signed-off-by: Alina (Xi) Li <Alina.Li@improving.com>
Signed-off-by: Alina (Xi) Li <Alina.Li@improving.com>
Signed-off-by: Alina (Xi) Li <Alina.Li@improving.com>
Signed-off-by: Alina (Xi) Li <Alina.Li@improving.com>
Signed-off-by: Alina (Xi) Li <Alina.Li@improving.com>
Signed-off-by: Alina (Xi) Li <Alina.Li@improving.com>
- Add type_error_code test case to errors.py
- Add Code to all_bson_types_distinct and all_bson_types_dedup in element_dedup.py
- Add __init__.py to setUnion directory
- Update expected sizes from 17 to 18

Signed-off-by: Alina (Xi) Li <Alina.Li@improving.com>
alinaliBQ added 4 commits June 2, 2026 14:06
Signed-off-by: Alina (Xi) Li <Alina.Li@improving.com>
Signed-off-by: Alina (Xi) Li <Alina.Li@improving.com>
Signed-off-by: Alina (Xi) Li <Alina.Li@improving.com>
Signed-off-by: Alina (Xi) Li <Alina.Li@improving.com>
Copy link
Copy Markdown
Contributor Author

@alinaliBQ alinaliBQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rishabh998 addressed comments. Regarding https://github.com/documentdb/functional-tests/pull/254/changes#r3343920563, I did not add any test since tests with different grouping is out of scope for accumulator tests

]

# Property [Grouping Key Variations]: $setUnion works with different _id
# grouping expressions.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added SETUNION_CHAINED_GROUP_TESTS for this

msg="$setUnion should work with compound _id grouping key",
),
]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having documents into different groups might be an out of scope as @eerxuan stated here: #257 (comment)
we are only testing the $setUnion accumulator, not broader document grouping

]

# Property [Object Element Deduplication]: objects are compared by structure
# including key order.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added SETUNION_MIXED_ELEMENT_TYPE_TESTS for this

msg="$setUnion should accept simple field path expression",
),
AccumulatorTestCase(
"expr_nested_field_path",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests SETUNION_MISSING_NESTED_PATH_TESTS for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compatibility test Compatibility test related enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants