Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/17008 )
Change subject: IMPALA-10463: Implement ds_theta_sketch() and ds_theta_estimate() functions ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/17008/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17008/2//COMMIT_MSG@28 PS2, Line 28: see IMPALA-10464. > Done Did you forgot to add this additional section to the commit msg? http://gerrit.cloudera.org:8080/#/c/17008/3/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/17008/3/be/src/exprs/aggregate-functions-ir.cc@1905 PS3, Line 1905: if (dst->len == sizeof(datasketches::theta_union)) { There is one more thing I don't understand here: Why do you have to branch based on the underlying type of 'dst'? For e.g. DsHllMerge() this wasn't needed because that function put always the HLL sketch into 'dst' and not the union object. http://gerrit.cloudera.org:8080/#/c/17008/3/be/src/exprs/aggregate-functions-ir.cc@1908 PS3, Line 1908: } else { A DCHECK would be nice in the else branch to verify that dst->len is sizeof(theta_sketch) It would also make the code a bit more readable IMO -- To view, visit http://gerrit.cloudera.org:8080/17008 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I14f24c16b815eec75cf90bb92c8b8b0363dcbfbc Gerrit-Change-Number: 17008 Gerrit-PatchSet: 3 Gerrit-Owner: Fucun Chu <[email protected]> Gerrit-Reviewer: Fucun Chu <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Mon, 15 Feb 2021 10:49:44 +0000 Gerrit-HasComments: Yes
