Fucun Chu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17153 )
Change subject: IMPALA-10558: Implement ds_theta_exclude() function ...................................................................... Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/17153/2/be/src/exprs/datasketches-functions-ir.cc File be/src/exprs/datasketches-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/17153/2/be/src/exprs/datasketches-functions-ir.cc@128 PS2, Line 128: datasketches::theta_a_not_b a_not_b; > nit: this comment is not needed as doesn't give extra info Done http://gerrit.cloudera.org:8080/#/c/17153/2/be/src/exprs/datasketches-functions-ir.cc@131 PS2, Line 131: if (!first_serialized_sketch.is_null && first_serialized_sketch.len > 0) { : if (!DeserializeDsSketch(first_serialized_sketch, &first_sketch_ptr)) { : LogSketchDeserializationError(ctx); : return StringVal::null(); : } : } : datasketches::theta_sketch::unique_ptr second_sketch_ptr; : if (!second_serialized_sketch.is_null && second_serialized_sketch.len > 0) { : if (!DeserializeDsSketch(second_serialized_sketch, &second_sketch_ptr)) { : > This part seems pretty identical to the section L141-150. Can you move it t Done http://gerrit.cloudera.org:8080/#/c/17153/2/be/src/exprs/datasketches-functions-ir.cc@155 PS2, Line 155: d::stringstream serialized_input > I'm not sure I understand the condition in this format :) Could you please function ref: https://en.cppreference.com/w/cpp/memory/unique_ptr/operator_bool, usage has been modified with reference to the example. http://gerrit.cloudera.org:8080/#/c/17153/2/be/src/exprs/datasketches-functions.h File be/src/exprs/datasketches-functions.h: http://gerrit.cloudera.org:8080/#/c/17153/2/be/src/exprs/datasketches-functions.h@73 PS2, Line 73: 'first_serialized_s > Could you mention both sketch params? Done http://gerrit.cloudera.org:8080/#/c/17153/2/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/17153/2/testdata/workloads/functional-query/queries/QueryTest/datasketches-theta.test@330 PS2, Line 330: When A is empty and B is > When A is empty and B is null. Done http://gerrit.cloudera.org:8080/#/c/17153/2/testdata/workloads/functional-query/queries/QueryTest/datasketches-theta.test@331 PS2, Line 331: select ds_theta_estimate(ds_theta_exclude(ds_theta_sketch(f2), null)) > Could you please add another test where A is null and B is empty? (the oppo Done http://gerrit.cloudera.org:8080/#/c/17153/2/testdata/workloads/functional-query/queries/QueryTest/datasketches-theta.test@332 PS2, Line 332: from functional_parquet.emptytable; > Another test would be where A and B are both empty. Done http://gerrit.cloudera.org:8080/#/c/17153/2/testdata/workloads/functional-query/queries/QueryTest/datasketches-theta.test@379 PS2, Line 379: i.ti i_ti, i.i i_i, i.bi i_bi, i.f i_f, i.d i_d, i.s i_s, i.c i_c, i.v i_v,i.nc i_nc, > I miss a test where the result of an a-not-b is a non-empty sketch (where t Done -- To view, visit http://gerrit.cloudera.org:8080/17153 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I05119fd8c652c07ff248a99e44b0da3541e46ca3 Gerrit-Change-Number: 17153 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: Sun, 14 Mar 2021 14:30:46 +0000 Gerrit-HasComments: Yes
