Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/16626 )
Change subject: IMPALA-10132 Implement ds_hll_estimate_bounds_as_string() function. ...................................................................... Patch Set 2: (9 comments) Thanks for taking care of this implementation! The patch is in quite a good shape I just had some minor comments. http://gerrit.cloudera.org:8080/#/c/16626/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16626/2//COMMIT_MSG@7 PS2, Line 7: IMPALA-10132 Implement ds_hll_estimate_bounds_as_string() function. Could you mention that there are 2 versions of this function. One that accepts a kappa parameter and one that doesn't. Also it would help to understand this feature to explain this kappa parameter here and it's purpose. http://gerrit.cloudera.org:8080/#/c/16626/2//COMMIT_MSG@15 PS2, Line 15: butwith typo http://gerrit.cloudera.org:8080/#/c/16626/2//COMMIT_MSG@16 PS2, Line 16: ds_kll_cdf_as_string wrong function name, guess copy-pasted from the CDF change :) http://gerrit.cloudera.org:8080/#/c/16626/2/be/src/exprs/datasketches-common.h File be/src/exprs/datasketches-common.h: http://gerrit.cloudera.org:8080/#/c/16626/2/be/src/exprs/datasketches-common.h@43 PS2, Line 43: DS_DEFAULR_KAPPA typo http://gerrit.cloudera.org:8080/#/c/16626/2/be/src/exprs/datasketches-functions.h File be/src/exprs/datasketches-functions.h: http://gerrit.cloudera.org:8080/#/c/16626/2/be/src/exprs/datasketches-functions.h@50 PS2, Line 50: disinct typo http://gerrit.cloudera.org:8080/#/c/16626/2/be/src/exprs/datasketches-functions.h@55 PS2, Line 55: static StringVal DsHllEstimateBoundsAsString(FunctionContext* ctx, I wouldn't write this whole comment for this function. I'd rather mention that this is similar to the function above with the exception of this kappa param (and would explain this param.) No strong feelings here though, it's up to you. http://gerrit.cloudera.org:8080/#/c/16626/2/testdata/workloads/functional-query/queries/QueryTest/datasketches-hll.test File testdata/workloads/functional-query/queries/QueryTest/datasketches-hll.test: http://gerrit.cloudera.org:8080/#/c/16626/2/testdata/workloads/functional-query/queries/QueryTest/datasketches-hll.test@291 PS2, Line 291: select ds_hll_estimate_bounds_as_string(ds_kll_sketch(cast(f2 as float))) from functional_parquet.emptytable; Could you wrap these lines into 90 chars. If the output of a query is too long then there is no need to wrap but the comments and the queries shouldn't be longer than 90 chars for more readability. http://gerrit.cloudera.org:8080/#/c/16626/2/testdata/workloads/functional-query/queries/QueryTest/datasketches-hll.test@313 PS2, Line 313: 4 Please add a test for fraction inputs as well. http://gerrit.cloudera.org:8080/#/c/16626/2/testdata/workloads/functional-query/queries/QueryTest/datasketches-hll.test@315 PS2, Line 315: UDF ERROR: Kappa may not be less than 1 or greater than 3. I'd change this error msg to explicitly say these 3 values that are allowed. With the current msg one might have the impression that e.g. 1.5 is also a valid input. -- To view, visit http://gerrit.cloudera.org:8080/16626 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I46bf8263e8fd3877a087b9cb6f0d1a2392bb9153 Gerrit-Change-Number: 16626 Gerrit-PatchSet: 2 Gerrit-Owner: Fucun Chu <chufu...@hotmail.com> Gerrit-Reviewer: Fucun Chu <chufu...@hotmail.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Tue, 27 Oct 2020 11:43:10 +0000 Gerrit-HasComments: Yes
