[ 
https://issues.apache.org/jira/browse/HIVE-1397?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12879054#action_12879054
 ] 

HBase Review Board commented on HIVE-1397:
------------------------------------------

Message from: "Mayank Lahiri" <mayank.lah...@facebook.com>


bq.  On None, John Sichi wrote:
bq.  > 
http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/exec/FunctionRegistry.java,
 line 346
bq.  > <http://review.hbase.org/r/181/diff/1/?file=1351#file1351line346>
bq.  >
bq.  >     Since eventually we would like to support histograms on non-numeric 
fields such as STRING, I think we should rename this one numeric_histogram 
(likewise for the Java class) to avoid confusion later when we have other 
algorithms.
bq.  >

It might seem a little odd since histograms are generally used as 
approximations of numerical distributions. I would suggest either (a) 
overloading histogram() to behave differently on STRING arguments (perhaps 
STRING arguments that cause a NumberFormatException), or (b) creating a 
factor_histogram() function for general strings. 

I could add in the code for computing frequencies of STRINGs quite easily, 
although there's no way to prevent it from choking if there are too many unique 
strings.


bq.  On None, John Sichi wrote:
bq.  > 
http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFHistogram.java,
 line 239
bq.  > <http://review.hbase.org/r/181/diff/1/?file=1352#file1352line239>
bq.  >
bq.  >     Shouldn't this method be private?

yes! thanks!


bq.  On None, John Sichi wrote:
bq.  > 
http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFHistogram.java,
 line 186
bq.  > <http://review.hbase.org/r/181/diff/1/?file=1352#file1352line186>
bq.  >
bq.  >     Formatting nitpick:  Here and elsewhere, use braces even for 
single-line blocks; we use the NeedBraces checkstyle rule to flag this.
bq.  >     
bq.  >     
http://stackoverflow.com/questions/382633/can-the-checkstyle-module-needbraces-work-with-nested-if-else-blocks
bq.  >

Will re-submit patch, thanks.


bq.  On None, John Sichi wrote:
bq.  > 
http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFHistogram.java,
 line 362
bq.  > <http://review.hbase.org/r/181/diff/1/?file=1352#file1352line362>
bq.  >
bq.  >     Under what conditions can this exception be encountered?  Shouldn't 
it be impossible since we already checked the type up front?

This could happen when a STRING row contains a non-numeric value. As a 
follow-up to my earlier comment, we could either drop the value or somehow 
"intelligently" switch to computing the histogram over strings instead of 
doubles.


bq.  On None, John Sichi wrote:
bq.  > 
http://svn.apache.org/repos/asf/hadoop/hive/trunk/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFHistogram.java,
 line 153
bq.  > <http://review.hbase.org/r/181/diff/1/?file=1352#file1352line153>
bq.  >
bq.  >     Is it possible to return an ARRAY<STRUCT{x DOUBLE; y DOUBLE}> 
instead?  That seems more natural (and compact) than a MAP<DOUBLE,DOUBLE>.
bq.  >     
bq.  >     But if you already got feedback that MAP is prefereable, ignore this 
comment.
bq.  >

Not a problem. I was using the map to avoid an extra level of indirection, and 
possibly to make it compatible with an explode() extension that explodes maps 
as well as arrays. 


- Mayank


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/181/#review224
-----------------------------------------------------------





> histogram() UDAF for a numerical column
> ---------------------------------------
>
>                 Key: HIVE-1397
>                 URL: https://issues.apache.org/jira/browse/HIVE-1397
>             Project: Hadoop Hive
>          Issue Type: New Feature
>          Components: Query Processor
>    Affects Versions: 0.6.0
>            Reporter: Mayank Lahiri
>            Assignee: Mayank Lahiri
>             Fix For: 0.6.0
>
>         Attachments: Histogram_quality.png.jpg, HIVE-1397.1.patch
>
>
> A histogram() UDAF to generate an approximate histogram of a numerical (byte, 
> short, double, long, etc.) column. The result is returned as a map of (x,y) 
> histogram pairs, and can be plotted in Gnuplot using impulses (for example). 
> The algorithm is currently adapted from "A streaming parallel decision tree 
> algorithm" by Ben-Haim and Tom-Tov, JMLR 11 (2010), and uses space 
> proportional to the number of histogram bins specified. It has no 
> approximation guarantees, but seems to work well when there is a lot of data 
> and a large number (e.g. 50-100) of histogram bins specified.
> A typical call might be:
> SELECT histogram(val, 10) FROM some_table;
> where the result would be a histogram with 10 bins, returned as a Hive map 
> object.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply via email to