dgraph
dgraph copied to clipboard
dgraph-io
fix(rollups): cherry-pick-7609
The old PR fixed an issue that arose after posting/list.go was significantly refactored when roaring bitmaps were incorporated. The previous team first implemented roaring and then wrote the sroar library to compress the posting lists. In 21.03 we are not using roaring nor sroar so this code should not be merged. However a new test was written that we can bring in.
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.
:white_check_mark: joshua-goldstein
:x: ahsanbarkati
You have signed the CLA already but the status is still pending? Let us recheck it.
Coverage remained the same at 37.099% when pulling 2b1b8f528baf69dab60db88603daf89c4bba19af on cherry-pick-7609 into 9b670174e6af72b773ae59350d7fa3f75e91c80f on main.
@joshua-goldstein To recap, this PR only introduces a test, right? And that test exercises existing functionality with a "multi-part" list, right?
@joshua-goldstein To recap, this PR only introduces a test, right? And that test exercises existing functionality with a "multi-part" list, right?
Correct, we are just introducing a test here. And yes, that is correct, there is some commentary here regarding what is happening during rollups. In particular they mention that the posting list will be split up into multiple lists if it gets too big.
I was hoping this test addition would increase the coverage %, but it's kind of odd to see the coverage % to stay the same. I did validate the same in the run logs of the test, and this test did run
=== RUN TestLargePlistSplit
[8248](https://github.com/dgraph-io/dgraph/actions/runs/3079955654/jobs/4976760244#step:10:8249)
list_test.go:1018: Num splits: 2
[8249](https://github.com/dgraph-io/dgraph/actions/runs/3079955654/jobs/4976760244#step:10:8250)
--- PASS: TestLargePlistSplit (1.73s)
fyi @kevinmingtarja
I was hoping this test addition would increase the coverage %, but it's kind of odd to see the coverage % to stay the same. I did validate the same in the run logs of the test, and this test did run
=== RUN TestLargePlistSplit [8248](https://github.com/dgraph-io/dgraph/actions/runs/3079955654/jobs/4976760244#step:10:8249) list_test.go:1018: Num splits: 2 [8249](https://github.com/dgraph-io/dgraph/actions/runs/3079955654/jobs/4976760244#step:10:8250) --- PASS: TestLargePlistSplit (1.73s)fyi @kevinmingtarja
Hi @skrdgraph , posting lists are integrated deeply into Dgraph and there are already many tests covering them. There are many cases that need to be covered but if the methods are already tested elsewhere then checking additional cases wouldn't add to the test coverage metric, correct? Of course the issue here is that "test coverage" is a rough metric.
I was hoping this test addition would increase the coverage %, but it's kind of odd to see the coverage % to stay the same. I did validate the same in the run logs of the test, and this test did run
=== RUN TestLargePlistSplit [8248](https://github.com/dgraph-io/dgraph/actions/runs/3079955654/jobs/4976760244#step:10:8249) list_test.go:1018: Num splits: 2 [8249](https://github.com/dgraph-io/dgraph/actions/runs/3079955654/jobs/4976760244#step:10:8250) --- PASS: TestLargePlistSplit (1.73s)fyi @kevinmingtarja
Hi @skrdgraph , posting lists are integrated deeply into Dgraph and there are already many tests covering them. There are many cases that need to be covered but if the methods are already tested elsewhere then checking additional cases wouldn't add to the test coverage metric, correct? Of course the issue here is that "test coverage" is a rough metric.
That makes sense, retesting a tested function/method - with another test - doesn't increase the number. I was under the assumption that this was a new test we were bringing in to test a component that was un-tested.