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 2: (9 comments) I'm still looking at the tests, just want to publish what I have so far. Overall the patch (and approach) looks good to me. 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: of a SampledNdvState with 'num_buckets' HLL bu > Good idea done. I did some more similar cleanup in this class. thanks, easier to follow. http://gerrit.cloudera.org:8080/#/c/8569/2/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/8569/2/be/src/exprs/aggregate-functions-ir.cc@1443 PS2, Line 1443: count IMO 'count' sounds a little generic and overlaps with total_count. May be rename them to "bucket_row_count" and "total_row_count" or something similar? http://gerrit.cloudera.org:8080/#/c/8569/2/be/src/exprs/aggregate-functions-ir.cc@1459 PS2, Line 1459: double sample_perc; static? http://gerrit.cloudera.org:8080/#/c/8569/2/be/src/exprs/aggregate-functions-ir.cc@1465 PS2, Line 1465: static int64_t GetStateSize(int64_t num_buckets) { Given num_buckets is always a const, may be make this a static const too like BUCKET size? static const in64_t STATE_SIZE = NUM_HLL_BUCKETS * BUCKET_SIZE ..... http://gerrit.cloudera.org:8080/#/c/8569/2/be/src/exprs/aggregate-functions-ir.cc@1530 PS2, Line 1530: ndv may be call ndv_estimate, just to be clear http://gerrit.cloudera.org:8080/#/c/8569/2/be/src/exprs/aggregate-functions-ir.cc@1552 PS2, Line 1552: int bucket_idx = (i + j) % SampledNdvState::NUM_HLL_BUCKETS; : merged_count += *state->GetCountPtr(bucket_idx); : counts[pidx] = merged_count; : StringVal hll = StringVal(state->GetHllPtr(bucket_idx), HLL_LEN); : HllMerge(ctx, hll, &merged_hll); : ndvs[pidx] = HllFinalEstimate(merged_hll.ptr, HLL_LEN); Wondering if we can avoid duplicates in data points since hll_to_merge for (i,j) = (j,i). Do they add any additional value? We could probably optimize the loop a little further to avoid that for (int i=0, i < NUM_HLL_BUCKETS; ++i) { for (int j=i, j < NUM_HLL_BUCKETS, ++j) { } } http://gerrit.cloudera.org:8080/#/c/8569/2/be/src/exprs/aggregate-functions-ir.cc@1563 PS2, Line 1563: int64_t max_count = counts[num_points - 1]; not sure I understand this, what invariant guarantees counts[num_points-1] is the max_count? 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: /// Returns true if fitting was successful, false otherwise. > Done here. I could not find another place, did I miss one? nope, just here. http://gerrit.cloudera.org:8080/#/c/8569/2/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/2/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@506 PS2, Line 506: SAMPED_NDV nit: typo -- 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: 2 Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Comment-Date: Wed, 22 Nov 2017 22:36:41 +0000 Gerrit-HasComments: Yes
