-
-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support 'AND (... OR ... OR ...)' queries #70
Conversation
@kelindar requesting review In particular, I understand that a goal of this library is to remove allocations in the transaction pipeline as much as possible - however, I don't see a way around it without allocating an arbitrary number of "bonus" index columns during txnPool creation and letting transactions use them as needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, but would be good to have more tests
txn.go
Outdated
} | ||
|
||
// allocate temp bitmap for calcs | ||
tmpMap := make(bitmap.Bitmap, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is iterating by chunk, we'd need a bitmap of size 256 (16,384/64), unless I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your bitmap package automatically grows the temp map - https://github.com/kelindar/bitmap/blob/2bbf5487c3a81312758c6b0e6d51caf56dff1b14/bitmap_amd64.go#L67
However, it's more beneficial for use cases above one full chunk to pre-allocate, so setting the initial size to 256 is better imo. It also helped me notice that I wasn't completely zeroing-out the temp map.
Let me know if there's a certain type of test in mind that I'm missing |
Looks like I spoke too soon - the test failure looks adjacent to the issue fixed in your previous PR (#69). I personally run these tests on an Intel MacBook Pro & have also created a test container with Ubuntu to better mock the Github CI, but am not able* to replicate the failure consistently. |
Should Line 517 in ff08c96
|
pre-alloc size 256 bitmap; make players test larger; clear entire tmpMap
760f87d
to
c56baaf
Compare
Introduces
WithUnion
, which operates similarly toUnion
, but will allocate a separate bitmap (1) for calculating the multi-OR before applying it to the txn's current index via an AND. This could be used to solve #55.I'd still like to clean up the bitmap comparison logic and extend the testing to use
players