You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Sean Hsuan-Yi Chu <hs...@usc.edu> on 2015/03/27 21:33:21 UTC
Review Request 32590: DRILL-2591: In UnionAllRecordBactch,
the mechansim to detect schema change is corrected
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32590/
-----------------------------------------------------------
Review request for drill and Aman Sinha.
Bugs: DRILL-2591.1
https://issues.apache.org/jira/browse/DRILL-2591.1
Repository: drill-git
Description
-------
DRILL-2591: In UnionAllRecordBactch, the mechansim to detect schema change is corrected
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java 806104a
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/FinalColumnReorderer.java 1aa033b
exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 520c204
exec/java-exec/src/test/resources/store/json/dateData.json PRE-CREATION
exec/java-exec/src/test/resources/store/json/timeStmpData.json PRE-CREATION
exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q18.tsv PRE-CREATION
Diff: https://reviews.apache.org/r/32590/diff/
Testing
-------
QA, unit all passed
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 32590: DRILL-2591: In UnionAllRecordBactch,
the mechansim to detect schema change is corrected
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On April 3, 2015, 12:15 a.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java, line 123
> > <https://reviews.apache.org/r/32590/diff/2/?file=912235#file912235line123>
> >
> > It's not clear to me why we have to check specifically for NumberFormatException here...this is just another type of IllegalArgumentException.
This one is for issue DRILL-2608.
This will give the user proper error message regarding implicit casting failure.
Would it be better to make it be another new patch ???
> On April 3, 2015, 12:15 a.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java, line 334
> > <https://reviews.apache.org/r/32590/diff/2/?file=912235#file912235line334>
> >
> > For schema equivalence why do we need a new method ? Did you try equals() ? Having a separate method can be prone to bugs later if it gets out of sync with attributes of the class.
Done!
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32590/#review78745
-----------------------------------------------------------
On April 3, 2015, 8:04 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32590/
> -----------------------------------------------------------
>
> (Updated April 3, 2015, 8:04 p.m.)
>
>
> Review request for drill and Aman Sinha.
>
>
> Bugs: DRILL-2591.1
> https://issues.apache.org/jira/browse/DRILL-2591.1
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2591: In UnionAllRecordBactch, the mechansim to detect schema change is corrected
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java 806104a
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/FinalColumnReorderer.java 1aa033b
> exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java fcf5c9f
> exec/java-exec/src/test/resources/store/json/dateData.json PRE-CREATION
> exec/java-exec/src/test/resources/store/json/timeStmpData.json PRE-CREATION
> exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q18.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/32590/diff/
>
>
> Testing
> -------
>
> QA, unit all passed
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 32590: DRILL-2591: In UnionAllRecordBactch,
the mechansim to detect schema change is corrected
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32590/#review78745
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java
<https://reviews.apache.org/r/32590/#comment127751>
It's not clear to me why we have to check specifically for NumberFormatException here...this is just another type of IllegalArgumentException.
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java
<https://reviews.apache.org/r/32590/#comment127765>
For schema equivalence why do we need a new method ? Did you try equals() ? Having a separate method can be prone to bugs later if it gets out of sync with attributes of the class.
- Aman Sinha
On April 1, 2015, 9:39 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32590/
> -----------------------------------------------------------
>
> (Updated April 1, 2015, 9:39 p.m.)
>
>
> Review request for drill and Aman Sinha.
>
>
> Bugs: DRILL-2591.1
> https://issues.apache.org/jira/browse/DRILL-2591.1
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2591: In UnionAllRecordBactch, the mechansim to detect schema change is corrected
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java 806104a
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/FinalColumnReorderer.java 1aa033b
> exec/java-exec/src/main/java/org/apache/drill/exec/record/BatchSchema.java da2e9eb
> exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 1ebb534
> exec/java-exec/src/test/resources/store/json/dateData.json PRE-CREATION
> exec/java-exec/src/test/resources/store/json/timeStmpData.json PRE-CREATION
> exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q18.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/32590/diff/
>
>
> Testing
> -------
>
> QA, unit all passed
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 32590: DRILL-2591: In UnionAllRecordBactch,
the mechansim to detect schema change is corrected
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32590/#review78899
-----------------------------------------------------------
Ship it!
Ship It!
- Aman Sinha
On April 4, 2015, 3:08 a.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32590/
> -----------------------------------------------------------
>
> (Updated April 4, 2015, 3:08 a.m.)
>
>
> Review request for drill and Aman Sinha.
>
>
> Bugs: DRILL-2591.1
> https://issues.apache.org/jira/browse/DRILL-2591.1
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2591: In UnionAllRecordBactch, the mechansim to detect schema change is corrected
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java 806104a
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/FinalColumnReorderer.java 1aa033b
> exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java fcf5c9f
> exec/java-exec/src/test/resources/store/json/dateData.json PRE-CREATION
> exec/java-exec/src/test/resources/store/json/timeStmpData.json PRE-CREATION
> exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q18.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/32590/diff/
>
>
> Testing
> -------
>
> QA, unit all passed
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 32590: DRILL-2591: In UnionAllRecordBactch,
the mechansim to detect schema change is corrected
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32590/
-----------------------------------------------------------
(Updated April 4, 2015, 3:08 a.m.)
Review request for drill and Aman Sinha.
Changes
-------
new patch
Bugs: DRILL-2591.1
https://issues.apache.org/jira/browse/DRILL-2591.1
Repository: drill-git
Description
-------
DRILL-2591: In UnionAllRecordBactch, the mechansim to detect schema change is corrected
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java 806104a
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/FinalColumnReorderer.java 1aa033b
exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java fcf5c9f
exec/java-exec/src/test/resources/store/json/dateData.json PRE-CREATION
exec/java-exec/src/test/resources/store/json/timeStmpData.json PRE-CREATION
exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q18.tsv PRE-CREATION
Diff: https://reviews.apache.org/r/32590/diff/
Testing
-------
QA, unit all passed
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 32590: DRILL-2591: In UnionAllRecordBactch,
the mechansim to detect schema change is corrected
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32590/
-----------------------------------------------------------
(Updated April 3, 2015, 8:04 p.m.)
Review request for drill and Aman Sinha.
Bugs: DRILL-2591.1
https://issues.apache.org/jira/browse/DRILL-2591.1
Repository: drill-git
Description
-------
DRILL-2591: In UnionAllRecordBactch, the mechansim to detect schema change is corrected
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java 806104a
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/FinalColumnReorderer.java 1aa033b
exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java fcf5c9f
exec/java-exec/src/test/resources/store/json/dateData.json PRE-CREATION
exec/java-exec/src/test/resources/store/json/timeStmpData.json PRE-CREATION
exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q18.tsv PRE-CREATION
Diff: https://reviews.apache.org/r/32590/diff/
Testing
-------
QA, unit all passed
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 32590: DRILL-2591: In UnionAllRecordBactch,
the mechansim to detect schema change is corrected
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32590/
-----------------------------------------------------------
(Updated April 1, 2015, 9:39 p.m.)
Review request for drill and Aman Sinha.
Changes
-------
new patch
Bugs: DRILL-2591.1
https://issues.apache.org/jira/browse/DRILL-2591.1
Repository: drill-git
Description
-------
DRILL-2591: In UnionAllRecordBactch, the mechansim to detect schema change is corrected
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java 806104a
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/FinalColumnReorderer.java 1aa033b
exec/java-exec/src/main/java/org/apache/drill/exec/record/BatchSchema.java da2e9eb
exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 1ebb534
exec/java-exec/src/test/resources/store/json/dateData.json PRE-CREATION
exec/java-exec/src/test/resources/store/json/timeStmpData.json PRE-CREATION
exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q18.tsv PRE-CREATION
Diff: https://reviews.apache.org/r/32590/diff/
Testing
-------
QA, unit all passed
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 32590: DRILL-2591: In UnionAllRecordBactch,
the mechansim to detect schema change is corrected
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On March 29, 2015, 5:28 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java, line 357
> > <https://reviews.apache.org/r/32590/diff/1/?file=908305#file908305line357>
> >
> > The error message says 'right input' - it should be 'left input'
Updated!
> On March 29, 2015, 5:28 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java, line 356
> > <https://reviews.apache.org/r/32590/diff/1/?file=908305#file908305line356>
> >
> > Same as above.
Same as above.
> On March 29, 2015, 5:28 p.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java, line 330
> > <https://reviews.apache.org/r/32590/diff/1/?file=908305#file908305line330>
> >
> > This does not look right...the check here is checking schema change status of the outgoing batch of UnionAll, wherease you are interested in the incoming batch.
A new approach is to memorize the schema for the first record batch (from the left and right sides). And just the schemas for the subsequent record batches with the memorized schema. If they are different, that means schema has been changed.
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32590/#review78163
-----------------------------------------------------------
On March 27, 2015, 8:33 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32590/
> -----------------------------------------------------------
>
> (Updated March 27, 2015, 8:33 p.m.)
>
>
> Review request for drill and Aman Sinha.
>
>
> Bugs: DRILL-2591.1
> https://issues.apache.org/jira/browse/DRILL-2591.1
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2591: In UnionAllRecordBactch, the mechansim to detect schema change is corrected
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java 806104a
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/FinalColumnReorderer.java 1aa033b
> exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 520c204
> exec/java-exec/src/test/resources/store/json/dateData.json PRE-CREATION
> exec/java-exec/src/test/resources/store/json/timeStmpData.json PRE-CREATION
> exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q18.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/32590/diff/
>
>
> Testing
> -------
>
> QA, unit all passed
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 32590: DRILL-2591: In UnionAllRecordBactch,
the mechansim to detect schema change is corrected
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32590/#review78163
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java
<https://reviews.apache.org/r/32590/#comment126651>
This does not look right...the check here is checking schema change status of the outgoing batch of UnionAll, wherease you are interested in the incoming batch.
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java
<https://reviews.apache.org/r/32590/#comment126652>
Same as above.
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java
<https://reviews.apache.org/r/32590/#comment126653>
The error message says 'right input' - it should be 'left input'
- Aman Sinha
On March 27, 2015, 8:33 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32590/
> -----------------------------------------------------------
>
> (Updated March 27, 2015, 8:33 p.m.)
>
>
> Review request for drill and Aman Sinha.
>
>
> Bugs: DRILL-2591.1
> https://issues.apache.org/jira/browse/DRILL-2591.1
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-2591: In UnionAllRecordBactch, the mechansim to detect schema change is corrected
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/union/UnionAllRecordBatch.java 806104a
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/FinalColumnReorderer.java 1aa033b
> exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 520c204
> exec/java-exec/src/test/resources/store/json/dateData.json PRE-CREATION
> exec/java-exec/src/test/resources/store/json/timeStmpData.json PRE-CREATION
> exec/java-exec/src/test/resources/testframework/testUnionAllQueries/q18.tsv PRE-CREATION
>
> Diff: https://reviews.apache.org/r/32590/diff/
>
>
> Testing
> -------
>
> QA, unit all passed
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>