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 36: (9 comments) mostly nits http://gerrit.cloudera.org:8080/#/c/15997/34//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15997/34//COMMIT_MSG@16 PS34, Line 16: > nit: extra white space ping http://gerrit.cloudera.org:8080/#/c/15997/34//COMMIT_MSG@35 PS34, Line 35: The enhancement involves both the front and the back end. : : In the frond end, a 2nd parameter in NDV() is allowed and verified. : In addition, the data type of the intermediate result in the : plan records the correct amount of memory needed. This is assisted : by the inclusion of additional template aggregate function objects : in the built-in database. : : In the back end, the current hardcoded precision of 10 is removed. The : HLL algorithm now works with the default, or any valid precision values. : The precision value is computed from the corresponding scale value : stored in the query plan. > I think u can remove this whole section. ping - you can add this as a comment in the JIRA instead. generally commit messages should focus on what and why vs. how - https://chris.beams.io/posts/git-commit/#why-not-how http://gerrit.cloudera.org:8080/#/c/15997/34//COMMIT_MSG@56 PS34, Line 56: Performance > would be good to do some quick sanity checks to make sure ndv performance d ping http://gerrit.cloudera.org:8080/#/c/15997/34//COMMIT_MSG@83 PS34, Line 83: : Change-Id: I48a4517bd0959f7021143073d37505a46c551a58 : : : : > Remove this, its too verbose. You paste a link to the google doc containing ping http://gerrit.cloudera.org:8080/#/c/15997/36//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15997/36//COMMIT_MSG@51 PS36, Line 51: 2. Added and ran a new regression test (test_ndv)) in TestAggregationQueries this applies to the rest of the commit message as well, but if you look at the commit message in gerrit (https://gerrit.cloudera.org/#/c/15997/36//COMMIT_MSG), notice a vertical dashed red line on the right. that line is basically the cutoff length for each line of the commit message. ideally each line in the commit message is only 72 characters or less. if you use vim to write commit messages, there are standard tools that will do this for you automatically (e.g. https://github.com/tpope/vim-fugitive) http://gerrit.cloudera.org:8080/#/c/15997/36/be/src/exprs/aggregate-functions-ir.cc File be/src/exprs/aggregate-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/15997/36/be/src/exprs/aggregate-functions-ir.cc@1473 PS36, Line 1473: nit: extra white space http://gerrit.cloudera.org:8080/#/c/15997/36/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/36/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java@889 PS36, Line 889: help is this suppose to be "helper"? http://gerrit.cloudera.org:8080/#/c/15997/36/tests/query_test/test_aggregation.py File tests/query_test/test_aggregation.py: http://gerrit.cloudera.org:8080/#/c/15997/36/tests/query_test/test_aggregation.py@281 PS36, Line 281: nit: extra space http://gerrit.cloudera.org:8080/#/c/15997/36/tests/query_test/test_aggregation.py@281 PS36, Line 281: Test two argument version of NDV() same comment as before, referring to the "two argument version of NDV()" is vague. it would better to refer to it as "the version of NDV() that accepts a scale value". -- 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: 36 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: Mon, 22 Jun 2020 20:53:47 +0000 Gerrit-HasComments: Yes
