Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15997 )
Change subject: IMPALA-2658: Extend the NDV function to accept a precision ...................................................................... Patch Set 20: (6 comments) I think there are a few comments that spilled through the cracks. Marked them with a 'ping'. http://gerrit.cloudera.org:8080/#/c/15997/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15997/7//COMMIT_MSG@7 PS7, Line 7: IMPALA-2658: Extend the NDV function to accept a precision > ping - I think this still needs to be addressed ping http://gerrit.cloudera.org:8080/#/c/15997/7//COMMIT_MSG@45 PS7, Line 45: select ndv(c_name, 1) "one", ndv(c_name, 2) two, ndv(c_name, 3) three, > All commits should have a section called "Testing:" that describe what test Done http://gerrit.cloudera.org:8080/#/c/15997/7/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/15997/7/be/src/exprs/aggregate-functions-ir.cc@102 PS7, Line 102: DCHECK_IN_RANGE > FYI DCHECKs are *not* run in release builds; they are only run in debug bui Done http://gerrit.cloudera.org:8080/#/c/15997/9/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/15997/9/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@592 PS9, Line 592: > it should be: ping http://gerrit.cloudera.org:8080/#/c/15997/9/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java File fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java: http://gerrit.cloudera.org:8080/#/c/15997/9/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@352 PS9, Line 352: > I think it can be more descriptive still. Something like HLL_UPDATE_SYMBOL_ ping http://gerrit.cloudera.org:8080/#/c/15997/9/tests/query_test/test_aggregation.py File tests/query_test/test_aggregation.py: http://gerrit.cloudera.org:8080/#/c/15997/9/tests/query_test/test_aggregation.py@318 PS9, Line 318: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10] > ahh I thought this was the same as the list on line 301, but they are diffe ping -- To view, visit http://gerrit.cloudera.org:8080/15997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I48a4517bd0959f7021143073d37505a46c551a58 Gerrit-Change-Number: 15997 Gerrit-PatchSet: 20 Gerrit-Owner: Qifan Chen <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Comment-Date: Thu, 11 Jun 2020 16:53:18 +0000 Gerrit-HasComments: Yes
