Conversation
| fn merge_secondary_time_range_if_exists( | ||
| splits: &[SplitMetadata], | ||
| ) -> Option<RangeInclusive<DateTime>> { | ||
| if splits |
There was a problem hiding this comment.
Couldn't we merge the two iteration to improve performances ?
There was a problem hiding this comment.
We can, but I would replace the whole thing with a for loop because it already feels like the limit of what is digest in terms of iterators 😄. This is really not on the critical path as it happens only once for every merge and the list size is typically 10.
rdettai-sk
force-pushed
the
secondary-time
branch
4 times, most recently
from
874527c to
c49ec16
Compare
rdettai-sk
force-pushed
the
secondary-time
branch
from
c49ec16 to
44b4207
Compare
| /// Retains only splits with a secondary time range end that is defined and | ||
| /// *less than or equal to* the provided value. If the secondary time range | ||
| /// end is not defined, falls back to the primary. |
There was a problem hiding this comment.
@Darkheir I changed this to fallback on the primary if the secondary is not defined. It is tested. Can you just proofread it? (given that it has the potential to wipe the whole data 😄 )
There was a problem hiding this comment.
Seems good to me. No need to fallback for the other cases ?
|
built into sekoia-20251218 https://github.com/SekoiaLab/platform/actions/runs/20342851868 |
2 participants
Description
Describe the proposed changes made in this PR.
How was this PR tested?
Describe how you tested this PR.