Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Hive >> mail # dev >> Review Request 14576: NOT expression doesn't handle nulls correctly.


Copy link to this message
-
Re: Review Request 14576: NOT expression doesn't handle nulls correctly.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14576/#review27306
-----------------------------------------------------------

ql/src/gen/vectorization/ExpressionTemplates/ColumnArithmeticColumn.txt
<https://reviews.apache.org/r/14576/#comment53132>

    This is a minor point, but I think setNumArguments is redundant. It can be inferred fromthe number of arguments passed to setArgumentTypes and setInputExpressionTypes.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorExpressionDescriptor.java
<https://reviews.apache.org/r/14576/#comment53137>

    Nice job encapsulating this in a class. That is better than exposing the bitmap externally.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorExpressionDescriptor.java
<https://reviews.apache.org/r/14576/#comment53134>

    Should say "number of" not "number if".
    
    Why assume max 3 arguments? Is it necessary to have a max? Please explain in the comment.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorExpressionDescriptor.java
<https://reviews.apache.org/r/14576/#comment53139>

    Nice.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorExpressionDescriptor.java
<https://reviews.apache.org/r/14576/#comment53147>

    Please add a comment to say what this does and how it works.
    
    Consider making this method static.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java
<https://reviews.apache.org/r/14576/#comment53151>

    Please add a comment to describe vMap
    
    Also, please add private keyword if it is private

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java
<https://reviews.apache.org/r/14576/#comment53160>

    Can you elaborate more here in the comments? I'm not sure int what situations filter mode is used here and why you add the SelectColumnIsTrue. Is it for NOT?
    
    Also, the coding standards as for a blank line before all comment lines.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java
<https://reviews.apache.org/r/14576/#comment53161>

    extra blan space after VectorExpression

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java
<https://reviews.apache.org/r/14576/#comment53172>

    add blank line before comments
    

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizationContext.java
<https://reviews.apache.org/r/14576/#comment53174>

    Please put a comment to explain this line (you are putting the outputCol always as the last arg...)

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/VectorizedExpressions.java
<https://reviews.apache.org/r/14576/#comment53150>

    Please add a comment to describe the purpose of this interface.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/FilterColAndScalar.java
<https://reviews.apache.org/r/14576/#comment53175>

    Please add a comment to describe the purpose of this class.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/FilterColAndScalar.java
<https://reviews.apache.org/r/14576/#comment53176>

    please add a comment to explain the logic of this evaluate() method

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/FilterScalarAndColumn.java
<https://reviews.apache.org/r/14576/#comment53177>

    please add comment to say the purpose of this class

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/FuncLongToString.java
<https://reviews.apache.org/r/14576/#comment53179>

    It'd be good to format this like the other getDescriptor methods are formatted, for consistency

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/FuncPowerLongToDouble.java
<https://reviews.apache.org/r/14576/#comment53180>

    is it the case that this method and the ISetDoubleArg interface are not needed for this class any more?

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/PosModDoubleToDouble.java
<https://reviews.apache.org/r/14576/#comment53191>

    Is this interface needed any more?

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/PosModLongToLong.java
<https://reviews.apache.org/r/14576/#comment53192>

    Is ISetLongArg need any more?

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/RoundWithNumDigitsDoubleToDouble.java
<https://reviews.apache.org/r/14576/#comment53193>

    is iSetLongArg needed here any more?

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/udf/VectorUDFAdaptor.java
<https://reviews.apache.org/r/14576/#comment53194>

    Did you do an end-to-end test with the UDF Adaptor to make sure it still works?
    
    The unit test should cover it though.

ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorMathFunctions.java
<https://reviews.apache.org/r/14576/#comment53207>

    I think this .setArg call is now redundant given the constructor change.

ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorMathFunctions.java
<https://reviews.apache.org/r/14576/#comment53209>

    remove commented line?

ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorMathFunctions.java
<https://reviews.apache.org/r/14576/#comment53210>

    remove commented line?

ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/TestVectorStringExpressions.java
<https://reviews.apache.org/r/14576/#comment53213>

    All the getBytes() calls should be getBytes("UTF-8").
    
    It may actually matter for the multiByte char cases.
- Eric Hanson
On Oct. 21, 2013, 6 p.m., Jitendra Pandey wrote: