Tamas Mate has posted comments on this change. ( http://gerrit.cloudera.org:8080/17131 )
Change subject: IMPALA-10538: [DOCS] Document the newly added argument ...................................................................... Patch Set 1: (4 comments) Hi Shajini, thank you for this doc update. I had a few observations/questions on the doc change and some on commit. Impala commit messages are in general complete sentences and describe what the commit contains. Unfortunately, this part does not have anchor, so can not link directly, but at the end of the 'Fix' there is a short paragraph about the styling. https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impala http://gerrit.cloudera.org:8080/#/c/17131/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17131/1//COMMIT_MSG@7 PS1, Line 7: argument Could you change this to "NDV parameter"? 'NDV' to be more specific and 'parameter' because the function's definition has parameters, these called arguments when referring to then when the function is called. http://gerrit.cloudera.org:8080/#/c/17131/1//COMMIT_MSG@10 PS1, Line 10: hm This line is a bit long. http://gerrit.cloudera.org:8080/#/c/17131/1/docs/topics/impala_ndv.xml File docs/topics/impala_ndv.xml: http://gerrit.cloudera.org:8080/#/c/17131/1/docs/topics/impala_ndv.xml@51 PS1, Line 51: <p> The argument <codeph>scale</codeph> must be an integer and can be in the range from 1 to 10 : and maps to a precision used by the HLL algorithm with the following mapping formula: </p> : : <p><codeblock>precision = scale + 8</codeblock></p> I think an example at the end of the page could greatly help how the scale parameter affects the end result of NDV. http://gerrit.cloudera.org:8080/#/c/17131/1/docs/topics/impala_ndv.xml@69 PS1, Line 69: Without the secondary argument, all the syntax and semantics of the NDV function are : preserved. The precision, which determines the total number of different estimators in the HLL : algorithm, will be still 10. This looks odd in a user documentation, we should probably just share a default value, if my understanding is right 10 is the default. -- To view, visit http://gerrit.cloudera.org:8080/17131 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec8007b79afac59cdfb3984bb111806213c21c77 Gerrit-Change-Number: 17131 Gerrit-PatchSet: 1 Gerrit-Owner: Shajini Thayasingh <sthayasi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com> Gerrit-Comment-Date: Sun, 07 Mar 2021 15:36:31 +0000 Gerrit-HasComments: Yes
