[WIP] fix: mongo query predicate edge case#388
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the MongoDB query-filter construction to handle an edge case in bounded-interval pairing and to merge overlapping intervals, while also introducing basic unit-test infrastructure and a first regression test for predicate pairing.
Changes:
- Introduces
MongoOpandMongoPredicateand updates query predicate construction to use them. - Updates the bounded-inequality pairing algorithm to avoid invalid “bounded” intervals (e.g.
> 5with<= 5) and to merge overlapping intervals. - Adds pytest configuration, a unit test for the pairing logic, CI to run unit tests, and local testing instructions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/openf1/util/test_predicate_pairing.py | Adds unit tests for bounded-inequality pairing and overlap merging behavior. |
| src/openf1/util/misc.py | Extends hash_obj to handle dataclass inputs (currently implemented incorrectly). |
| src/openf1/util/db.py | Adds MongoOp/MongoPredicate, updates query predicate generation, pairing/merging logic, and session lookups. |
| src/openf1/services/query_api/query_params.py | Switches comparison-to-Mongo operators toward enums and updates filter conversion (currently mismapped). |
| pyproject.toml | Adds pytest discovery configuration. |
| CONTRIBUTING.md | Documents how to run tests locally. |
| .github/workflows/tests.yml | Adds a CI job to run unit tests with pytest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Args: | ||
| predicates: A list of inequality predicates without duplicates (i.e. no two predicates can have the same op and value). | ||
| Predicates have a MongoDB operator such as "$eq", "$gt", "$gte", "$lt", or "$lte", and a numeric value (e.g. int, float, datetime). | ||
|
|
||
| Returns: | ||
| A list of predicate pairs where the first predicate of the pair represents a lower bound ("$gt", "$gte"), | ||
| the second predicate of the pair represents an upper bound ("$lt", "$lte"), | ||
| and the value difference among all pairs is minimized (i.e. the value difference in each pair is as small as possible). | ||
| and the second predicate of the pair represents an upper bound ("$lt", "$lte"). | ||
|
|
||
| Examples: | ||
| [] --> [] | ||
| [{"$gt": 5}] --> [] | ||
| [{"$gt": 10}, {"$lt": 5}] --> [] | ||
| [{"$gt": 5}, {"$lt": 10}] --> [({"$gt": 5}, {"$lt": 10})] | ||
| [{"$gt": 5}, {"$lt": 10}, {"$lt": 12}] --> [({"$gt": 5}, {"$lt": 10})] | ||
| [{"$gt": 5}, {"$lt": 10}, {"$gt": 8}, {"$lt": 12}] --> [({"$gt": 5}, {"$lt": 10}), ({"$gt": 8}, {"$lt": 12})] | ||
| [{"$eq": 5}] --> [] | ||
| [{"$gt": 5}] --> [] (unbounded interval) | ||
| [{"$gt": 10}, {"$lt": 5}] --> [] (unbounded intervals) | ||
| [{"$gt": 5}, {"$lt": 10}] --> [({"$gt": 5}, {"$lt": 10})] (bounded interval) | ||
| [{"$gt": 5}, {"$lt": 10}, {"$lt": 12}] --> [({"$gt": 5}, {"$lt": 10})] (bounded interval with unbounded interval) | ||
| [{"$gt": 5}, {"$lt": 10}, {"$gt": 8}, {"$lt": 12}] --> [({"$gt": 5}, {"$lt": 12})] (bounded intervals merged due to overlap) |
There was a problem hiding this comment.
The docstring examples for _get_bounded_inequality_predicate_pairs still show the old { "$gt": 5 } dict-style predicates, but the function now accepts MongoPredicate objects. Updating these examples (and the Args description) would prevent confusion for future readers.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Thanks a lot @JeffreyJPZ, great work! |
|
Hi @JeffreyJPZ, |
|
Hi @br-g, sorry about that! I'll try and resolve the merge conflicts, fix some of the documentation and maybe add a couple tests for |
No problem, take the time you need! I just wanted to check on the status |
|
@br-g it should be ready now for a first pass! |
|
Hi @JeffreyJPZ, Sorry for the delay getting back to you, and thanks for your patience! This is really impressive work. The more I dig into this code, the more I appreciate how much complexity you've untangled. I did find a few issues while testing locally:
Would you have time to look into these? No rush! Let me know what works. |
|
Hi @br-g, Thanks for the in-depth review, this merging logic is pretty tricky 😅 - I'll have a look at it later this week or next week. |
Changes
position > 5and upper boundposition <= 5, this does not result in a "bounded" interval.MongoOpenum andMongoPredicatedataclass to get rid of repeated literalsI would ideally like to add a few more unit tests before this gets done.
Feedback would be appreciated, I could also break this up into multiple PRs if necessary.