Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/16370 )
Change subject: IMPALA-10108: Implement ds_kll_stringify function ...................................................................... Patch Set 3: (5 comments) For me reading this code review doesn't really reveal what this new function returns exactly. Neither the commit msg neither the tests give any details about the content of the sketch summary. http://gerrit.cloudera.org:8080/#/c/16370/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16370/3//COMMIT_MSG@10 PS3, Line 10: stringified format Could you mention what does this stringified format look like? What information does it contain? http://gerrit.cloudera.org:8080/#/c/16370/3/be/src/exprs/datasketches-functions-ir.cc File be/src/exprs/datasketches-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/16370/3/be/src/exprs/datasketches-functions-ir.cc@144 PS3, Line 144: FunctionContext* ctx nit: this param would fit in the previous line, right? http://gerrit.cloudera.org:8080/#/c/16370/3/be/src/exprs/datasketches-functions.h File be/src/exprs/datasketches-functions.h: http://gerrit.cloudera.org:8080/#/c/16370/3/be/src/exprs/datasketches-functions.h@109 PS3, Line 109: a nit: an http://gerrit.cloudera.org:8080/#/c/16370/3/be/src/exprs/datasketches-functions.h@110 PS3, Line 110: static StringVal DsKllStringify( Is this meant to be in the private section? http://gerrit.cloudera.org:8080/#/c/16370/3/testdata/workloads/functional-query/queries/QueryTest/datasketches-kll.test File testdata/workloads/functional-query/queries/QueryTest/datasketches-kll.test: http://gerrit.cloudera.org:8080/#/c/16370/3/testdata/workloads/functional-query/queries/QueryTest/datasketches-kll.test@604 PS3, Line 604: row_regex: .*### KLL sketch summary:.*### End sketch summary.* What is the content of the sketch summary? Can't you assert on anything that is inside of the sketch summary? -- To view, visit http://gerrit.cloudera.org:8080/16370 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I97f654a4838bf91e3e0bed6a00d78b2c7aa96f75 Gerrit-Change-Number: 16370 Gerrit-PatchSet: 3 Gerrit-Owner: Adam Tamas <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 26 Aug 2020 14:21:56 +0000 Gerrit-HasComments: Yes
