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

Switch to Threaded View
Hive, mail # dev - Review Request: Column Column, and Column Scalar vectorized execution tests


Copy link to this message
-
Re: Review Request: Column Column, and Column Scalar vectorized execution tests
Eric Hanson 2013-05-14, 18:13

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

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestCodeGen.java
<https://reviews.apache.org/r/11133/#comment42356>

    missing apache license

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestCodeGen.java
<https://reviews.apache.org/r/11133/#comment42357>

    classes need javadoc comment at beginning

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestCodeGen.java
<https://reviews.apache.org/r/11133/#comment42354>

    See java coding style guide
    http://www.oracle.com/technetwork/java/codeconv-138413.html
    
    Differences for Hive: indent 2 chars not 4, and 100 char line length limit

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestCodeGen.java
<https://reviews.apache.org/r/11133/#comment42355>

    need comment to explain purpose and contents of this matrix

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnColumnFilterVectorExpressionEvaluation.txt
<https://reviews.apache.org/r/11133/#comment42337>

    All in all this is a really nice approach. Nice clean test all on one page, fully templatized.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnColumnFilterVectorExpressionEvaluation.txt
<https://reviews.apache.org/r/11133/#comment42332>

    BATCH_SIZE is optional in the constructors now and not normally recommended. The default is recommended. But that should not really matter here for correctness.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnColumnFilterVectorExpressionEvaluation.txt
<https://reviews.apache.org/r/11133/#comment42334>

    Style issue with curly braces. Run ant checkstyle.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnColumnFilterVectorExpressionEvaluation.txt
<https://reviews.apache.org/r/11133/#comment42336>

    selectedInUse can be true even of all rows have been filtered out
    
    If all rows qualify against a filter, an optimization is to set selectedInUse back to false. But this is not mandatory for correctness.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnColumnOperationVectorExpressionEvaluation.txt
<https://reviews.apache.org/r/11133/#comment42341>

    you have quit a bit of extra trailing white space that shows up red in the review tool. I think you can set eclipse to get rid of it but I'm not sure how.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnColumnOperationVectorExpressionEvaluation.txt
<https://reviews.apache.org/r/11133/#comment42342>

    style guide says put blank after comma

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnScalarFilterVectorExpressionEvaluation.txt
<https://reviews.apache.org/r/11133/#comment42344>

    what if input has noNulls? Then you should not look at isNull array.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnScalarFilterVectorExpressionEvaluation.txt
<https://reviews.apache.org/r/11133/#comment42346>

    if the optimization is used whereby if every row qualifies, you set selectedInUse to false, this could give a false failure

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnScalarFilterVectorExpressionEvaluation.txt
<https://reviews.apache.org/r/11133/#comment42339>

    See earlier comment on this.

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnScalarOperationVectorExpressionEvaluation.txt
<https://reviews.apache.org/r/11133/#comment42351>

    test .txt templates need apache license

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnScalarOperationVectorExpressionEvaluation.txt
<https://reviews.apache.org/r/11133/#comment42350>

    scalarValue set but never used

ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnScalarOperationVectorExpressionEvaluation.txt
<https://reviews.apache.org/r/11133/#comment42347>

    not supposed to look at isNull if noNulls is set for vector

ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/gen/TestColumnColumnFilterVectorExpressionEvaluation.java
<https://reviews.apache.org/r/11133/#comment42353>

    Why not just used VectorizedRowBatch.DEFAULT_SIZE? is there a reason?
- Eric Hanson
On May 14, 2013, 12:34 a.m., tony murphy wrote: