IPED
IPED copied to clipboard
sepinf-inc
Bitmap bookmarks
Implementations of IBookmark and IMultiBookmark that uses RoaringBitmap internally. The old implementation was kept to keep compatibility with saved bookmarks. So, the new implementation can be loaded from old saved bookmarks.
This should be pulled after, or concurrently with, #39, as improvement are done, creating a new Filter Interface for filters that exposes their internal bitmaps.
Also, timeline was changed to get directly this bookmark bitmap to make intersection operations with current result set and time period bitmaps, improving event occurrence counting times.
I have identified some conflict with code merged by #1771. I will check and solve, but I'm warning here to let the reviewer double check it.
@patrickdalla, after #1613 was merged, many merge conflicts appeared here. I'll wait your return to resolve them, then I'll proceed with code review and tests. Not sure, but it may be better to create a new PR without the commits related to the Filter Manager and with just the specific commits related to the bitmap bookmarks.
Thank you @patrickdalla for resolving the conflicts! Seems much better now, all commits related to the Filter Manager don't appear anymore.
By the way, seems some commits may be related to possible issues in the Timeline chart. Do they happen just on this PR or also on last released 4.1.5 version? If also on 4.1.5, could you create specific issues describing them, so we can properly list the fixes in the release notes?
I have made changes on timeline chart so they can take advantage of roaringbitmaps from bookmarks, when splitting the charts by bookmark.
Em ter., 19 de mar. de 2024, 12:18, Luis Filipe Nassif < @.***> escreveu:
Thank you @patrickdalla https://github.com/patrickdalla for resolving the conflicts! Seems much better now, all commits related to the Filter Manager don't appear anymore.
By the way, seems some commits are be related to possible issues in the Timeline chart. Do they happen just on this PR or also on last released 4.1.5 version? If also on 4.1.5, could you create specific issues describing them, so we can properly list the fixes in the release notes?
— Reply to this email directly, view it on GitHub https://github.com/sepinf-inc/IPED/pull/1648#issuecomment-2007608929, or unsubscribe https://github.com/notifications/unsubscribe-auth/AG247S4DSRIVYJE7N5QZ5QLYZBQOFAVCNFSM6AAAAAAXCPPVQOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBXGYYDQOJSHE . You are receiving this because you were mentioned.Message ID: @.***>
@patrickdalla, just opening the case, the "Clear Filters" button is shown without any filter applied (even before my fixes). I'm still not sure how to fix it, if you have time and could help fixing it, I would appreciate, thanks.
Hi @lfcnassif . I was putting the "all items" roaringbitmap array as an initial "filter", so the other filters would be applied against it. So IPED was counting it as a filter. I refactored so this initial roaringbitmap array is initialized just in the first Bitmap Filter addition. Please, check if it is right now, as maybe I have not tested enough.
I think I finished my review. But since this is a very sensitive change that may affect the user analysis results (bookmarks), I would like a second review by another one. @wladimirleite, if and when you have some available time, could you take a second look at this?
I think I finished my review. But since this is a very sensitive change that may affect the user analysis results (bookmarks), I would like a second review by another one. @wladimirleite, if and when you have some available time, could you take a second look on this?
Sure, I can do that. I need to finish a couple of somewhat urgent things, but I will review this in the next days.
Just a heads up here, I am testing this PR and so far everything is working fine. It is going a it slowly, as I have to finish other stuff I am woking on, but I expect to conclude my tests/review until the end of this week.
During the last few days I have been working on a real case, which I processed with this PR and master. I use the case processed with this PR as the "main case", replicating bookmark-related operations in the other case. I just generated the report with both versions, and the results seem fine. I also took a look at code changes. It was not simple to follow, as there were many modifications, but I didn't spot anything suspicious. That was pretty much what I had in mind to check this PR. It seems fine to me.
Great! Thank you very much @wladimirleite for helping testing this! There was no need to test this with a real case, but that is great!
I'm merging to master. Thank you @patrickdalla for this PR again!
This morning working with the same real case I mentioned above, I started to get some exceptions when creating new bookmarks. I am still trying to confirm/isolate the issue and find out what is going on.
Exception in thread "Thread-36112" java.lang.NullPointerException
at iped.engine.data.BitmapBookmarks.addBookmark(BitmapBookmarks.java:188)
at iped.engine.data.MultiBitmapBookmarks.addBookmark(MultiBitmapBookmarks.java:144)
at iped.engine.data.MultiBitmapBookmarks.addBookmark(MultiBitmapBookmarks.java:134)
at iped.app.ui.BookmarksManager$2.run(BookmarksManager.java:574)