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 3: Code-Review+1 (3 comments) LGTM. 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@1465 PS2, Line 1465: static int64_t GetStateSize() { > Can't do that because sizeof(SampledNdvState) fails since SampledNdvState i oops, yes. http://gerrit.cloudera.org:8080/#/c/8569/2/be/src/exprs/aggregate-functions-ir.cc@1552 PS2, Line 1552: tringVal merged_hll(merged_hll_data, HLL_LEN); : int64_t merged_count = 0; : for (int j = 0; j < SampledNdvState::NUM_HLL_BUCKETS; ++j) { : int bucket_idx = (i + j) % SampledNdvState::NUM_HLL_BUCKETS; : merged_count += *state->GetCountPtr(bucket_idx); : counts[pidx] = merged_count; > I expanded the comment here to explain and justify. Ah, I misread the part where we do HllMerge(ctx, hll, &merged_hll). I read it as HllMerge(ctx, hll, &src_hll[i]). It makes sense now, thanks. I agree the chance of dupes is pretty less and that also explains the max_count invariant. My bad. 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 > The value passed to this agg fn is independent of how much data is scanned. Makes sense, I too think the same. Thanks for explaining. -- 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-Comment-Date: Tue, 28 Nov 2017 22:58:29 +0000 Gerrit-HasComments: Yes
