Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15746 )
Change subject: IMPALA-9631: Import HLL functionality from DataSketches ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/15746/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15746/2//COMMIT_MSG@7 PS2, Line 7: IMPALA-9631: Import HLL functionality from DataSketches Do you plan to merge this patch as it is? I would consider it more of a POC until the HLL function becomes usable from the outside. Generally it is better to not keep reviews open for a long time and merge things in smaller increments, but I think that this is a different case as nearly all changes are in new files so there is no danger of conflicts. http://gerrit.cloudera.org:8080/#/c/15746/2//COMMIT_MSG@9 PS2, Line 9: This patch imports the functionality needed for HLL approximate algorithm Can you add this information in a README.md? To the datasketches folder? http://gerrit.cloudera.org:8080/#/c/15746/2//COMMIT_MSG@11 PS2, Line 11: be/src/thirdparty/datasketches. Note, that the original structure of files nit: wrap at 72 http://gerrit.cloudera.org:8080/#/c/15746/2/be/src/exprs/datasketches-test.cc File be/src/exprs/datasketches-test.cc: http://gerrit.cloudera.org:8080/#/c/15746/2/be/src/exprs/datasketches-test.cc@27 PS2, Line 27: algorith typo: algorithm http://gerrit.cloudera.org:8080/#/c/15746/2/be/src/exprs/datasketches-test.cc@38 PS2, Line 38: const char* file_name1 = "hll_sketch1.bin"; : const char* file_name2 = "hll_sketch2.bin"; I would prefer to use stringstream instead of files to make this faster/avoid side effects. -- To view, visit http://gerrit.cloudera.org:8080/15746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ca8e77dcbb6b6c3b1e3bca7ab57cb7d3c018bbf Gerrit-Change-Number: 15746 Gerrit-PatchSet: 2 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 17 Apr 2020 14:18:11 +0000 Gerrit-HasComments: Yes
