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 2: (9 comments) 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 r Done http://gerrit.cloudera.org:8080/#/c/8569/2/be/src/exprs/aggregate-functions-ir.cc@1459 PS2, Line 1459: double sample_perc; > static? This is the local state of a single aggregation instance, so static does not make sense. 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 li Can't do that because sizeof(SampledNdvState) fails since SampledNdvState is considered to be (size not known). This is the error: error: invalid application of ‘sizeof’ to incomplete type ‘impala::SampledNdvState’ sizeof(SampledNdvState) + 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 Done 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 expanded the comment here to explain and justify. Not sure I follow your point generation method. I think in my method only the last data point is guaranteed to be a duplicate (which is a good thing, now explained in comment). Others may or may not be duplicates (probably not duplicates though). Consider this example input and my point generation method: input counts: 1, 2, 3, 4 counts of 1st rolling window: 1, 3, 6, 10 counts of 2nd rolling window 2, 5, 9, 10 counts of 3rd rolling window 3, 7, 8, 10 counts of 4th rolling window 4, 5, 7, 10 For each "count" value above, we mere the corresponding HLL intermediates to get an NDV estimate for that subset of the data. Note that the merged NDVs corresponding to the same "count" value are likely to be different because they represent different subsets of the data. 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] Added comment. 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 Done http://gerrit.cloudera.org:8080/#/c/8569/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/8569/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@2063 PS2, Line 2063: 0.0 > Wondering if 0.0 is a valid sample %. What do we scan in that case? Do we a The value passed to this agg fn is independent of how much data is scanned. You can think of the sample percent as an "extrapolation factor". We will eventually combine TABLESAMPLE and SAMPLED_NDV() in COMPUTE STATS and there the effective sampling rate will be plugged into the SAMPLED_NDV() function. But for now, let's view SAMPLED_NDV() as a standalone aggregate function. I think 0.0 is a valid value. It will give you the estimated NDV at x=0. Not sure how useful that is, but I don't think it's invalid (0 is a valid "sampling ratio"). I'm not sure it's worth special casing 0, what do you think? http://gerrit.cloudera.org:8080/#/c/8569/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@2066 PS2, Line 2066: for (Column col: allScalarTypes.getColumns()) { > -ve test cases on non scalar columns, just for completeness? Maybe I'm misunderstanding but we don't support Exprs on non-scalar types. Even if we have nested collections, we must explode them to scalars. -- 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: Thu, 23 Nov 2017 01:08:29 +0000 Gerrit-HasComments: Yes
