Fix/dercbot 1676 fix search inconsistency #1969
Conversation
- Add dialogActivityFrom/To parameters to DialogsSearchQuery and DialogReportQuery - Implement period overlap filtering logic in UserTimelineMongoDAO using MongoDB aggregation - Add MongoDB aggregation DSL (MongoAggregationDSL) for readable date filtering expressions - Add unit tests for date filtering (creation date and activity period overlap) - Add service tests for DialogsSearchQuery mapping to DialogReportQuery
- Add toJsonSafe() helper function to handle Instant serialization in tests - Replace all toJson() calls with toJsonSafe() in test assertions - Fixes CodecConfigurationException when serializing Documents containing Instant values
- Rename MongoAggregationDSL.kt to MongoAgg.kt (single top-level declaration) - Rename Agg object to MongoAgg - Replace wildcard imports with explicit imports in UserTimelineMongoDAO - Replace wildcard imports and merge consecutive KDocs in UserTimelineMongoDAOTest - Apply ktlint auto-format fixes
assouktim
left a comment
assouktim
left a comment
There was a problem hiding this comment.
Thanks for the update. A few changes to consider.
| * )) | ||
| * ``` | ||
| */ | ||
| object MongoAgg { |
There was a problem hiding this comment.
This utility class is not necessary in this case.
The functional requirement is simply to answer the following question:
“Is there at least one action in all the dialogue stories whose date meets the condition?”
The current code already meets this requirement in a simple, readable, and efficient way, without introducing additional logic to process arrays via $reduce, $cond, or $ifNull.
Stories.actions.date lt dialogCreationDateTo?.toInstant(): This filter, for example, applies directly to the field contained in the stories and actions arrays.
MongoDB will therefore match documents that contain at least one action whose date meets the condition.
There was a problem hiding this comment.
This PR addresses two distinct filtering requirements:
1. Filter by Dialog Creation Date (dialogCreationDateFrom / dialogCreationDateTo)
Requirement: Filter dialogs where the creation date (= the date of the first/oldest action) falls within a specified period.
Condition: creationDateFrom <= min(actions.date) <= creationDateTo
For the upper bound (dialogCreationDateTo), the simple approach works because if min(date) <= toDate, then at least one action exists with date <= toDate.
However, for the lower bound (dialogCreationDateFrom), the simple approach fails:
Counter-example:
- Dialog with actions at dates:
[2025-01-01, 2025-01-15, 2025-01-30] - Filter:
dialogCreationDateFrom = 2025-01-10(dialogs created from January 10th)
| Approach | Result | Correct? |
|---|---|---|
Simple (Stories.actions.date gte 2025-01-10) |
Matches (Jan 15th action satisfies) | Wrong |
Aggregation (min(actions.date) >= 2025-01-10) |
No match (Jan 1st < Jan 10th) | Correct |
The dialog was created on January 1st, so it should NOT be returned when filtering for dialogs created from January 10th.
The Agg class with $reduce/$min is required for this filter.
2. Filter by Dialog Activity (dialogActivityFrom / dialogActivityTo)
Requirement: Filter dialogs that had activity during the specified period, meaning:
- At least one action on or after the lower bound (
activityFrom) - At least one action before the upper bound (
activityTo)
These two conditions do not need to be on the same action.
Condition:
∃ action >= activityFrom(at least one action after the lower bound)∃ action < activityTo(at least one action before the upper bound)
Mathematical equivalence:
| Statement | Equivalent to |
|---|---|
∃ action >= activityFrom |
max(actions.date) >= activityFrom |
∃ action < activityTo |
min(actions.date) < activityTo |
This means both approaches produce the same result for the activity filter:
// Simple approach
Stories.actions.date gte activityFrom
Stories.actions.date lt activityTo
// Aggregation approach (current implementation)
Agg.filterByPeriodOverlap(...) // max >= from AND min < toYou are correct that the simple approach would work for this filter. I chose the aggregation approach for consistency with the creation date filter and to make the intent explicit (computing min/max), but both are functionally equivalent.
Summary
| Filter | Requirement | Simple approach works? | Agg required? |
|---|---|---|---|
| dialogCreationDate | min(actions.date) within period |
No (fails for from) |
Yes |
| dialogActivity | ∃ action >= from AND ∃ action < to | Yes | No (but used for consistency) |
There was a problem hiding this comment.
I prefer the simplest option.
| } else { | ||
| Stories.actions.date lt dialogCreationDateTo?.toInstant() | ||
| }, | ||
| buildDialogCreationDateFilter( |
There was a problem hiding this comment.
The gte and lte operators must be used to avoid the strict nature of the condition, especially since filtering is now performed at specific times.
There was a problem hiding this comment.
You're right, there is an inconsistency in the date filtering operators across the codebase.
Current state of date filters
| Filter | from operator |
to operator |
Interval |
|---|---|---|---|
| annotationCreationDate | gt (exclusive) |
lt (exclusive) |
(from, to) |
| dialogCreationDate | gte (inclusive) |
lte (inclusive) |
[from, to] |
| dialogActivity | gte (inclusive) |
lt (exclusive) |
[from, to) |
The problem
Three different conventions for three different filters:
- Annotations: Open interval
(from, to)— both bounds are exclusive - Creation date: Closed interval
[from, to]— both bounds are inclusive - Activity: Half-open interval
[from, to)— lower bound inclusive, upper bound exclusive
This is indeed confusing and error-prone, especially when filtering at specific timestamps.
Proposed harmonization
I suggest adopting [from, to] (closed interval with gte/lte) for all date filters:
- More intuitive for users: "from January 1st to January 31st" includes both dates
- Consistent behavior across all filters
- Avoids edge cases where exact timestamp matches are unexpectedly excluded
I will update:
annotationCreationDate: changegt→gteandlt→ltedialogActivity: changelt→lte(and updateMongoAgg.filterByPeriodOverlap)
Do you agree with this approach, or would you prefer a different convention (e.g., half-open [from, to) everywhere)?
There was a problem hiding this comment.
Yes, a closed interval with gte/lte for all date filters.
| import kotlin.text.take | ||
| import kotlin.text.trim | ||
| import kotlin.to | ||
| import kotlin.with |
There was a problem hiding this comment.
We discussed this block:
if (from == null) null else DialogCol_.LastUpdateDate gt from?.toInstant(), if (to == null) null else DialogCol_.LastUpdateDate lt to?.toInstant(),
we said that it was redundant and, above all, useless because the from and to contain dead code. Please double-check, and if that's correct, remove them from the query and filtering.
| UserTimelineMongoDAO.loadWithoutDialogs("namespace", newId).playerId, | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
I will validate this test later on. I am waiting for your feedback on my first review, as it will significantly impact this unit test.
| val dialogCreationDateTo: ZonedDateTime? = null, | ||
| /** | ||
| * Filter dialogs by activity period overlap. | ||
| * A dialog is included if its activity period (from first to last action) overlaps the filter range. |
There was a problem hiding this comment.
I'm not sure that's right. Normally, at least one action must be within the range, not all of the actions in the dialogue.
A dialogue must be included if at least one action is within the range.
There was a problem hiding this comment.
see first comment.
|
ktlint fails, try reformatting your code by doing: mvn clean antrun:run@ktlint-format |
- Update documentation to clarify activity filter behavior - Remove dead code (from/to fields in DialogReportQuery) - Fix ktlint formatting issues in tests
- Change annotationCreationDate filter: gt→gte, lt→lte - Change dialogActivity filter: lt→lte for toDate - Update documentation to reflect inclusive bounds - Add unit tests to verify inclusivity of date filter bounds
2 participants
This pull request introduces new support for filtering dialogs by their activity period overlap, in addition to creation date, and provides a reusable DSL for building MongoDB aggregation expressions. It also adds comprehensive unit tests to ensure correct mapping of date parameters in dialog search queries.
Dialog filtering enhancements:
dialogActivityFromanddialogActivityTofields to bothDialogsSearchQueryandDialogReportQueryto allow filtering dialogs based on whether their activity period (from first to last action) overlaps a given range. This is documented and mapped in the conversion method. [1] [2] [3]MongoDB aggregation improvements:
MongoAggobject providing a DSL for building MongoDB aggregation expressions, including helpers for date aggregation in nested arrays and for filtering by creation date or activity period overlap. This makes aggregation queries more readable and maintainable.Testing:
DialogSearchServiceTestto verify that all date filtering parameters (creation and activity periods) are correctly mapped fromDialogsSearchQuerytoDialogReportQuery, including handling of nulls and combinations.Code organization:
UserTimelineMongoDAO.ktto include explicit Kotlin standard library imports for better clarity and maintainability.