Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/17186 )
Change subject: IMPALA-10581: Implement ds_theta_intersect_f() function ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/17186/4/be/src/exprs/datasketches-functions-ir.cc File be/src/exprs/datasketches-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/17186/4/be/src/exprs/datasketches-functions-ir.cc@194 PS4, Line 194: /// Returns true when intersection_sketch is updated Please add more comment about the use cases when this could return false. and also please mention that this might write error log as well. Additionally this could also be added to the header file as well in my opinion. http://gerrit.cloudera.org:8080/#/c/17186/4/be/src/exprs/datasketches-functions-ir.cc@195 PS4, Line 195: thera typo: theta http://gerrit.cloudera.org:8080/#/c/17186/4/be/src/exprs/datasketches-functions-ir.cc@223 PS4, Line 223: // Result This comment is not needed http://gerrit.cloudera.org:8080/#/c/17186/4/testdata/workloads/functional-query/queries/QueryTest/datasketches-theta.test File testdata/workloads/functional-query/queries/QueryTest/datasketches-theta.test: http://gerrit.cloudera.org:8080/#/c/17186/4/testdata/workloads/functional-query/queries/QueryTest/datasketches-theta.test@560 PS4, Line 560: ==== I miss 2 tests here: 1: Where the non-empty input sketches are distinct and the result is empyt. 2: When the inputs aren't the same but has some (but not all) items common. Could you please add coverage for these as well? -- To view, visit http://gerrit.cloudera.org:8080/17186 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I335eada00730036d5433775cfe673e0e4babaa01 Gerrit-Change-Number: 17186 Gerrit-PatchSet: 4 Gerrit-Owner: Fucun Chu <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Thu, 25 Mar 2021 09:16:34 +0000 Gerrit-HasComments: Yes
