> 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 > >
