[kudu-CR] KUDU-2038-p1: Introduce CRoaring

["helifu (Code Review)" <ge...@cloudera.org>]

helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12588 )

Change subject: KUDU-2038-p1: Introduce CRoaring
......................................................................


Patch Set 2:

(6 comments)

Sorry for the late reply.

http://gerrit.cloudera.org:8080/#/c/12588/2/cmake_modules/FindCRoaring.cmake
File cmake_modules/FindCRoaring.cmake:

http://gerrit.cloudera.org:8080/#/c/12588/2/cmake_modules/FindCRoaring.cmake@18
PS2, Line 18: # - Find CRoaring (roaring/roaring.hh, libroaring.a, and libroaring.so)
> Are all of these necessary? E.g. could we get by with just the static lib?
yep, the static lib is enough.


http://gerrit.cloudera.org:8080/#/c/12588/2/src/kudu/tablet/index/croaring-test.cc
File src/kudu/tablet/index/croaring-test.cc:

PS2: 
> nit: I think this makes more sense put into the /util directory, given ther
Done


http://gerrit.cloudera.org:8080/#/c/12588/2/src/kudu/tablet/index/croaring-test.cc@37
PS2, Line 37:      TEST_TYPE_SINGLE,
            :      TEST_TYPE_PAIR,
            :      TEST_TYPE_SKIP
> nit: add comments documenting what these are.
Done


http://gerrit.cloudera.org:8080/#/c/12588/2/src/kudu/tablet/index/croaring-test.cc@66
PS2, Line 66: t.n_bytes_array_containers +
            :            t.n_bytes_run_containers +
            :            t.n_bytes_bitset_containers;
> How is this different from getSizeInBytes?
"t.n_bytes_xxx_containers" means number of allocated bytes, and "getSizeInBytes" means how many bytes are required to serialize the object(roaring). They are different.


http://gerrit.cloudera.org:8080/#/c/12588/2/src/kudu/tablet/index/croaring-test.cc@119
PS2, Line 119:   printf("          Single         Pair        Skip\n");
> nit: I think there's a DataTable class you can use that helps print things 
Done


http://gerrit.cloudera.org:8080/#/c/12588/2/src/kudu/tablet/index/croaring-test.cc@120
PS2, Line 120: *6*/
> Remove this.
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/12588
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I878da34e388fb402c52c51364a68e738f785673d
Gerrit-Change-Number: 12588
Gerrit-PatchSet: 2
Gerrit-Owner: helifu <hz...@corp.netease.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: helifu <hz...@corp.netease.com>
Gerrit-Comment-Date: Wed, 17 Apr 2019 13:01:50 +0000
Gerrit-HasComments: Yes