kgyrtkirk commented on PR #3317: URL: https://github.com/apache/hive/pull/3317#issuecomment-1142141281
PR-1824 has nothing to do with datasketches; I don't know how you followed it's conventions but you might end up in trouble...because DS also has a HLL implementation...wouldn't that conflict with the existing one? note: PR-1824 named the file `VectorUDAFComputeBitVector.txt` and internally named the method `compute_bit_vector_hll` ; I think the class name should have contained the `Hll` keyword I think that the file name `VectorUDAFComputeKLL.txt` is not connected at all to the `ds_kll_sketch` function its about to vectorize...and as such its a bit confusing.... The current implementation doesn't really look forward: I think we have 20 *sketch* function from datasketches already exposed as inside Hive which could be vectorized; I think they are behind the same api cover...so just vectorizing the KLL one without any sight forward and taking "ideas" from the old hll codepath doesn't seem the best idea to me... ``` grep ^ds_ ql/src/test/results/clientpositive/llap/show_functions.q.out|grep _sketch$ ``` no need to do everything in 1 patch - but this is pretty much just copy-pasting the existing hll txtfile substituted to kll here and there...so we should do that 20 times? > For instance, you seem to be suggesting to remove all helper classes/methods etc I don't think those changes neccessary in the *metastore* for a vectorization of this function? HIVE-26221 is something which have changes - but has no real end-user accessible value - and as such I don't think its ready. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
