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 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/exprs/aggregate-functions-ir.cc@1443 PS3, Line 1443: bucket_ > I don't understand why we have "bucket" as a prefix here. Both the row_cou Done http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/exprs/aggregate-functions-ir.cc@1456 PS3, Line 1456: existing issues with passing constant arguments to all : /// aggregation phases > what does that mean? is there a JIRA we can just reference? Yes. IMPALA-6179. Added. http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/exprs/aggregate-functions-ir.cc@1469 PS3, Line 1469: int64_t* GetCountPtr(int bucket_idx) { : return reinterpret_cast<int64_t*>(GetBucketPtr(bucket_idx)); : } : : uint8_t* GetHllPtr(int bucket_idx) { : return GetBucketPtr(bucket_idx) + sizeof(int64_t); : } : : uint8_t* GetBucketPtr(int bucket_idx) { : return reinterpret_cast<uint8_t*>(this) + : sizeof(SampledNdvState) + bucket_idx * BUCKET_SIZE; : } > given that NUM_HLL_BUCKETS is predetermined, why not just use an array: Much better. Done. http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/exprs/aggregate-functions-ir.cc@1500 PS3, Line 1500: void AggregateFunctions::SampledNdvUpdate(FunctionContext* ctx, const T& src, > I think a quick comment explaining what we're doing here would be helpful, Done http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/exprs/aggregate-functions-ir.cc@1503 PS3, Line 1503: state->total_row_count % SampledNdvState::NUM_HLL_BUCKETS; > I wonder if picking a random bucket is better? We talked a bit about this. Interesting idea that I'll investigate. Ideally I'd not want to hold up this patch since the current simpler approach has shown good results and already took a long time to tune. http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/exprs/aggregate-functions-ir.cc@1533 PS3, Line 1533: int64_t counts[num_points]; : memset(counts, 0, num_points * sizeof(int64_t)); > how about: Done http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/exprs/aggregate-functions-ir.cc@1542 PS3, Line 1542: diminishing returns from creating additional data points > not sure what that means. do you mean additional to 'num_points'? i.e. are Yes. Rephrased to clarify. http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/exprs/aggregate-functions.h File be/src/exprs/aggregate-functions.h: http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/exprs/aggregate-functions.h@209 PS3, Line 209: x/y > I think using a notation like "(x, y)" maybe clearer since this sounds like Done http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/util/mpfit-util.h File be/src/util/mpfit-util.h: http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/util/mpfit-util.h@74 PS3, Line 74: /// Human-readable name of this function. Used for debugging. > should any of these members be private? Oops. Yes, all of them. Done. http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/util/mpfit-util.cc File be/src/util/mpfit-util.cc: http://gerrit.cloudera.org:8080/#/c/8569/3/be/src/util/mpfit-util.cc@33 PS3, Line 33: function > but what is this function suppose to do? Does it make sense to call it a " Added comment but did not rename. Not sure if "FittingFunction" would be clearer. This user supplied function is a little weird imo. Happy to rename if you prefer FittingFunction(), lmk. http://gerrit.cloudera.org:8080/#/c/8569/3/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/3/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@519 PS3, Line 519: children_.get(1) > samplePerc? Done http://gerrit.cloudera.org:8080/#/c/8569/3/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@524 PS3, Line 524: children_.get(1) > samplePerc? Done. Setting child 1 for clarify but it's not necessary since the cast modifies the literal in-place. http://gerrit.cloudera.org:8080/#/c/8569/3/tests/query_test/test_aggregation.py File tests/query_test/test_aggregation.py: http://gerrit.cloudera.org:8080/#/c/8569/3/tests/query_test/test_aggregation.py@337 PS3, Line 337: * sample_perc > even with the comment, I'm not sure what's going on here. Tried to clarify in comment. Lmk if still unclear. http://gerrit.cloudera.org:8080/#/c/8569/3/tests/query_test/test_aggregation.py@339 PS3, Line 339: def __appx_equals(self, a, b, diff_perc): > a quick comment for this would be helpful. Done -- 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: 3 Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Comment-Date: Wed, 06 Dec 2017 06:57:52 +0000 Gerrit-HasComments: Yes
