Fucun Chu has posted comments on this change. ( http://gerrit.cloudera.org:8080/16959 )
Change subject: IMPALA-10440: Import Theta functionality from DataSketches ...................................................................... Patch Set 3: (4 comments) Thanks for the review! Addressed the comments. http://gerrit.cloudera.org:8080/#/c/16959/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16959/2//COMMIT_MSG@15 PS2, Line 15: This affects files originated from hll/, kll/ and the > also files were changed in hll/ kll/ dirs Done http://gerrit.cloudera.org:8080/#/c/16959/2//COMMIT_MSG@22 PS2, Line 22: https://github.com/apache/datasketches-cpp/tree/b2f749ed5ce6ba650f4259602b133c310c3a5ee4 > Could you include a link that point to the particular git commit of the rep Done http://gerrit.cloudera.org:8080/#/c/16959/2/be/src/exprs/datasketches-test.cc File be/src/exprs/datasketches-test.cc: http://gerrit.cloudera.org:8080/#/c/16959/2/be/src/exprs/datasketches-test.cc@176 PS2, Line 176: for (int key = 0; key < 100000; key++) sketch1.update(key); > nit: drop the extra empty line Done http://gerrit.cloudera.org:8080/#/c/16959/2/be/src/thirdparty/datasketches/README.md File be/src/thirdparty/datasketches/README.md: http://gerrit.cloudera.org:8080/#/c/16959/2/be/src/thirdparty/datasketches/README.md@1 PS2, Line 1: CPC, HLL, KLL > nit: please use a comma between these names. Additionally, could you please Done -- To view, visit http://gerrit.cloudera.org:8080/16959 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8485d6829f50b130c84ec8bef0a4b5895255ba6c Gerrit-Change-Number: 16959 Gerrit-PatchSet: 3 Gerrit-Owner: Fucun Chu <chufu...@hotmail.com> Gerrit-Reviewer: Fucun Chu <chufu...@hotmail.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Thu, 21 Jan 2021 06:05:34 +0000 Gerrit-HasComments: Yes
