drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sean Hsuan-Yi Chu" <hsua...@usc.edu>
Subject Re: Review Request 31707: DRILL-2207: New Union-All Implementation
Date Sat, 07 Mar 2015 01:32:36 GMT


> On March 6, 2015, 10:45 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java,
line 62
> > <https://reviews.apache.org/r/31707/diff/2/?file=885179#file885179line62>
> >
> >     This should ideally be just UnionAll. Is an import missing ?

Initially, there is another class (the Runtime Code Generation template for union all) also
called unionall, so package name is used to differentiate; Now I change the former one to
UnionAller.


> On March 6, 2015, 10:45 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java,
line 241
> > <https://reviews.apache.org/r/31707/diff/2/?file=885179#file885179line241>
> >
> >     Won't this convert to nullable even if the desired output type is non-nullable
? 
> >     Also, I think in general most of this logic for deciding the casting should
live outside of the UnionAll code in some common utility class such that it is used by multiple
operators.

Move this part of code to ExpressionTreeMaterializer; after all, addCastExpression() method
has been in this this class.


> On March 6, 2015, 10:45 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java,
line 288
> > <https://reviews.apache.org/r/31707/diff/2/?file=885179#file885179line288>
> >
> >     Can you run some tests with TPCH SF1 such that we are testing with larger numbers
of record batches ?  Also test a case where left side has 0 rows, right side has non-zero.

1. [TPCH SF1] A bunch of tests were tested on TPCH SF1; Looked fine
2. [Either side with 0 rows] Worked as we expected; To sum up again, no matter which side
has no record, the output schema always follows the left side


> On March 6, 2015, 10:45 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java,
line 361
> > <https://reviews.apache.org/r/31707/diff/2/?file=885179#file885179line361>
> >
> >     You should identify whether the schema change occurred on left or right input.
Pls change the error to a clearer one, e.g "Schema change detected in <left/right> input
of Union-All.  This is not currently supported."

More appropriate exception message is given


> On March 6, 2015, 10:45 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/PreProcessLogicalRel.java,
line 150
> > <https://reviews.apache.org/r/31707/diff/2/?file=885182#file885182line150>
> >
> >     Error message should be clearer: "Union-all over schema-less tables must specify
the columns explicitly"

More appropriate exception message is given


> On March 6, 2015, 10:45 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/FinalColumnReorderer.java,
line 51
> > <https://reviews.apache.org/r/31707/diff/2/?file=885183#file885183line51>
> >
> >     Is this going to force a Project on the right side even if the column names,
ordinals and number of columns are the same as the ones on the left side ?  I think we should
avoid passing a 'force' flag here.

This field is actually necessary. I took it off by adding another if-statement before calling
this method


- Sean Hsuan-Yi


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


On March 7, 2015, 12:49 a.m., Sean Hsuan-Yi Chu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31707/
> -----------------------------------------------------------
> 
> (Updated March 7, 2015, 12:49 a.m.)
> 
> 
> Review request for drill, Aman Sinha and Jinfeng Ni.
> 
> 
> Bugs: DRILL-2207
>     https://issues.apache.org/jira/browse/DRILL-2207
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> DRILL-2207: New Union-All Implementation
> 
> 
> Diffs
> -----
> 
>   exec/java-exec/src/main/java/org/apache/drill/exec/expr/ExpressionTreeMaterializer.java
3565bf4 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java
99aec92 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAller.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllerTemplate.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java
270462b 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/PreProcessLogicalRel.java
4c9d301 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/FinalColumnReorderer.java
60a9e4b 
>   exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/parser/UnsupportedOperatorsVisitor.java
dcd5ebf 
>   exec/java-exec/src/main/java/org/apache/drill/exec/record/VectorContainer.java baf74b1

>   exec/java-exec/src/main/java/org/apache/drill/exec/resolver/TypeCastRules.java f5b0de4

>   exec/java-exec/src/test/java/org/apache/drill/TestExampleQueries.java 225b21e 
>   exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 36b062b 
>   exec/java-exec/src/test/resources/store/text/data/t.json PRE-CREATION 
>   exec/java-exec/src/test/resources/testframework/testExampleQueries/testAggregationOnUnionAllOperator/q1.tsv
PRE-CREATION 
>   exec/java-exec/src/test/resources/testframework/testExampleQueries/testAggregationOnUnionAllOperator/q2.tsv
PRE-CREATION 
>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q1.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q10.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q11.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q12.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q13.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q14.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q15.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q16.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q17.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q2.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q3.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q4.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q5.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q6.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q6_1.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q7.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q8.tsv PRE-CREATION

>   exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q9.tsv PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/31707/diff/
> 
> 
> Testing
> -------
> 
> Design Doc can be found from:
> https://issues.apache.org/jira/browse/DRILL-2207
> 
> Unit, Customers, TPCH passed
> waiting for Functional...
> 
> 
> Thanks,
> 
> Sean Hsuan-Yi Chu
> 
>


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