You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Gabor Kaszab (Code Review)" <ge...@cloudera.org> on 2021/03/02 10:13:03 UTC

[Impala-ASF-CR] IMPALA-10520: Implement ds theta intersect() function

Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/17088 )

Change subject: IMPALA-10520: Implement ds_theta_intersect() function
......................................................................


Patch Set 3: Code-Review+1

(2 comments)

Thanks for this patch! In overall this looks great, I just had some minor comments.

http://gerrit.cloudera.org:8080/#/c/17088/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/17088/3//COMMIT_MSG@14
PS3, Line 14: an
nit: not needed


http://gerrit.cloudera.org:8080/#/c/17088/3/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/17088/3/testdata/workloads/functional-query/queries/QueryTest/datasketches-theta.test@271
PS3, Line 271: and checks if the intersection
             : # produces the same result as if these sketches were used separately to get the estimates
Could you add tests that cover the second part of this sentence so that we can sew what ds_theta_intersect() gives when processing the sketches separately (and to see if they in fact match with the results of this test)?



-- 
To view, visit http://gerrit.cloudera.org:8080/17088
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I80e68c2151c4604f0386d3dfb004c82b10293f97
Gerrit-Change-Number: 17088
Gerrit-PatchSet: 3
Gerrit-Owner: Fucun Chu <ch...@hotmail.com>
Gerrit-Reviewer: Fucun Chu <ch...@hotmail.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Mar 2021 10:13:03 +0000
Gerrit-HasComments: Yes