hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Eric Hanson" <eh...@microsoft.com>
Subject Re: Review Request: Column Column, and Column Scalar vectorized execution tests
Date Tue, 14 May 2013 18:13:12 GMT

-----------------------------------------------------------
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:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/11133/
> -----------------------------------------------------------
> 
> (Updated May 14, 2013, 12:34 a.m.)
> 
> 
> Review request for hive, Jitendra Pandey, Eric Hanson, Sarvesh Sakalanaga, and Remus
Rusanu.
> 
> 
> Description
> -------
> 
> This patch adds Column Column, and Column Scalar vectorized execution tests. These tests
are generated in parallel with the vectorized expressions. The tests focus is on validating
the column vector and the vectorized row batch metadata regarding nulls, repeating, and selection.
> 
> Overview of Changes:
> 
> CodeGen.java:
> + joinPath, getCamelCaseType, readFile and writeFile made static for use in TestCodeGen.java.
> + filter types now specify null as their output type rather than "doesn't matter" to
make detection for test generation easier.
> + support for test generation added.
> 
> TestCodeGen.java & Templates: 
>      TestClass.txt
>      TestColumnColumnFilterVectorExpressionEvaluation.txt,
>      TestColumnColumnOperationVectorExpressionEvaluation.txt,
>      TestColumnScalarFilterVectorExpressionEvaluation.txt,
>      TestColumnScalarOperationVectorExpressionEvaluation.txt
> +This class is mutable and maintains a hashmap of TestSuiteClassName to test cases. The
tests cases are added over the course of vectorized expressions class generation, with test
classes being outputted at the end. For each column vector (inputs and/or outputs) a matrix
of pairwise covering Booleans is used to generate test cases across nulls and repeating dimensions.
Based on the input column vector(s) nulls and repeating states the states of the output column
vector (if there is one) is validated, along with the null vector. For filter operations the
selection vector is validated against the generated data. Each template corresponds to a class
representing a test suite.
> 
> VectorizedRowGroupUtil.java
> +added methods generateLongColumnVector and generateDoubleColumnVector for generating
the respective column vectors with optional nulls and/or repeating values.
> 
> 
> This addresses bug HIVE-4553.
>     https://issues.apache.org/jira/browse/HIVE-4553
> 
> 
> Diffs
> -----
> 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/CodeGen.java
53d9a7a 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestClass.txt
PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestCodeGen.java
PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnColumnFilterVectorExpressionEvaluation.txt
PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnColumnOperationVectorExpressionEvaluation.txt
PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnScalarFilterVectorExpressionEvaluation.txt
PRE-CREATION 
>   ql/src/java/org/apache/hadoop/hive/ql/exec/vector/expressions/templates/TestColumnScalarOperationVectorExpressionEvaluation.txt
PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/gen/TestColumnColumnFilterVectorExpressionEvaluation.java
PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/gen/TestColumnColumnOperationVectorExpressionEvaluation.java
PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/gen/TestColumnScalarFilterVectorExpressionEvaluation.java
PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/vector/expressions/gen/TestColumnScalarOperationVectorExpressionEvaluation.java
PRE-CREATION 
>   ql/src/test/org/apache/hadoop/hive/ql/exec/vector/util/VectorizedRowGroupGenUtil.java
8a07567 
> 
> Diff: https://reviews.apache.org/r/11133/diff/
> 
> 
> Testing
> -------
> 
> generated tests, and ran them.
> 
> 
> Thanks,
> 
> tony murphy
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message