Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/8569 )
Change subject: IMPALA-5310: Part 2: Add SAMPLED_NDV() function. ...................................................................... Patch Set 1: (7 comments) I have a bunch of nits. Although I follow the basic flow of the patch, I think I need to spend more time by running some examples to get a deeper understanding, as I'm not fully familiar with how the curve fitting math works. http://gerrit.cloudera.org:8080/#/c/8569/1/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/8569/1/be/src/exprs/aggregate-functions-ir.cc@1464 PS1, Line 1464: sizeof(int64_t) + AggregateFunctions::HLL_LEN) maybe factor this out into a GetSingleBucketSize() or something for readability? (here and below) http://gerrit.cloudera.org:8080/#/c/8569/1/be/src/exprs/aggregate-functions-ir.cc@1469 PS1, Line 1469: sizeof(SampledNdvState) + bucket_idx * (sizeof(int64_t) + AggregateFunctions::HLL_LEN)); nit: longline http://gerrit.cloudera.org:8080/#/c/8569/1/be/src/util/mpfit-util.h File be/src/util/mpfit-util.h: http://gerrit.cloudera.org:8080/#/c/8569/1/be/src/util/mpfit-util.h@50 PS1, Line 50: bool LmsFit(const double* xs, const double* ys, int num_points); WARN_UNUSED_RESULT, here and elsewhere http://gerrit.cloudera.org:8080/#/c/8569/1/be/src/util/mpfit-util.h@82 PS1, Line 82: mp_result result_; Looks like this struct has a bunch of ptrs, whats the lifecycle of those? http://gerrit.cloudera.org:8080/#/c/8569/1/be/src/util/mpfit-util.h@90 PS1, Line 90: const double* ys_; DISALLOW_COPY_ASSIGN http://gerrit.cloudera.org:8080/#/c/8569/1/be/src/util/mpfit-util.cc File be/src/util/mpfit-util.cc: http://gerrit.cloudera.org:8080/#/c/8569/1/be/src/util/mpfit-util.cc@29 PS1, Line 29: num_params_(num_params), : fn_(fn), : num_points_(0), : xs_(nullptr), : ys_(nullptr) { nit: I think we could condense them into fewer lines (other places in the codebase use multiple args per line) http://gerrit.cloudera.org:8080/#/c/8569/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/8569/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@507 PS1, Line 507: && children_.size() == 2 Isn't this more like a preconditions check inside the if {}? -- To view, visit http://gerrit.cloudera.org:8080/8569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia51d56ee67ec6073e92f90bebb4005484138b820 Gerrit-Change-Number: 8569 Gerrit-PatchSet: 1 Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Comment-Date: Thu, 16 Nov 2017 08:28:37 +0000 Gerrit-HasComments: Yes
