Alex Behm 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) 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 readabi Good idea done. I did some more similar cleanup in this class. 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 Done 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 Done here. I could not find another place, did I miss one? 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? Added comment. http://gerrit.cloudera.org:8080/#/c/8569/1/be/src/util/mpfit-util.h@90 PS1, Line 90: const double* ys_; > DISALLOW_COPY_ASSIGN That's needed for putting into std::vector. 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 c Done 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 {}? No. We're parsing a generic FunctionCallExpr that can be invoked with any number of args and we want to return a "Could not resolve function" error when SAMPLED_NDV() is invoked with an invalid number. That's done in L527. Added comment. -- 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: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Comment-Date: Tue, 21 Nov 2017 19:03:34 +0000 Gerrit-HasComments: Yes
