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/04/06 18:45:48 UTC
Review Request 32886: DRILL-2639: Planner bug -
RelOptPlanner.CannotPlanExceptio
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32886/
-----------------------------------------------------------
Review request for drill and Aman Sinha.
Bugs: DRILL-2639
https://issues.apache.org/jira/browse/DRILL-2639
Repository: drill-git
Description
-------
At planning for Union-All, block only the cases where implicit casting cannot resolve an output type
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java aba6022
exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillUnionRelBase.java 932aa76
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java 796f0f7
exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java fee1d6a
Diff: https://reviews.apache.org/r/32886/diff/
Testing
-------
QA, unit
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 32886: DRILL-2639: Planner bug -
RelOptPlanner.CannotPlanExceptio
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On April 10, 2015, 3:30 a.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java, lines 74-75
> > <https://reviews.apache.org/r/32886/diff/2/?file=922124#file922124line74>
> >
> > Pls use SqlTypeName.getName() instead of toString() even though getName() returns the toString value.
Done
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32886/#review79631
-----------------------------------------------------------
On April 10, 2015, 12:52 a.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32886/
> -----------------------------------------------------------
>
> (Updated April 10, 2015, 12:52 a.m.)
>
>
> Review request for drill and Aman Sinha.
>
>
> Bugs: DRILL-2639
> https://issues.apache.org/jira/browse/DRILL-2639
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> At planning for Union-All, block only the cases where implicit casting cannot resolve an output type
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/types/Types.java 015ee90
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java aba6022
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillUnionRelBase.java 932aa76
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java 796f0f7
> exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 7033013
>
> Diff: https://reviews.apache.org/r/32886/diff/
>
>
> Testing
> -------
>
> QA, unit
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 32886: DRILL-2639: Planner bug -
RelOptPlanner.CannotPlanExceptio
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32886/#review79631
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java
<https://reviews.apache.org/r/32886/#comment129131>
Pls use SqlTypeName.getName() instead of toString() even though getName() returns the toString value.
- Aman Sinha
On April 10, 2015, 12:52 a.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32886/
> -----------------------------------------------------------
>
> (Updated April 10, 2015, 12:52 a.m.)
>
>
> Review request for drill and Aman Sinha.
>
>
> Bugs: DRILL-2639
> https://issues.apache.org/jira/browse/DRILL-2639
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> At planning for Union-All, block only the cases where implicit casting cannot resolve an output type
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/types/Types.java 015ee90
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java aba6022
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillUnionRelBase.java 932aa76
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java 796f0f7
> exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 7033013
>
> Diff: https://reviews.apache.org/r/32886/diff/
>
>
> Testing
> -------
>
> QA, unit
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 32886: DRILL-2639: Planner bug -
RelOptPlanner.CannotPlanExceptio
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32886/#review79704
-----------------------------------------------------------
Ship it!
Ship It!
- Aman Sinha
On April 10, 2015, 4:31 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32886/
> -----------------------------------------------------------
>
> (Updated April 10, 2015, 4:31 p.m.)
>
>
> Review request for drill and Aman Sinha.
>
>
> Bugs: DRILL-2639
> https://issues.apache.org/jira/browse/DRILL-2639
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> At planning for Union-All, block only the cases where implicit casting cannot resolve an output type
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/types/Types.java 015ee90
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java aba6022
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillUnionRelBase.java 932aa76
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java 796f0f7
> exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 7033013
>
> Diff: https://reviews.apache.org/r/32886/diff/
>
>
> Testing
> -------
>
> QA, unit
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 32886: DRILL-2639: Planner bug -
RelOptPlanner.CannotPlanExceptio
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32886/
-----------------------------------------------------------
(Updated April 10, 2015, 4:31 p.m.)
Review request for drill and Aman Sinha.
Changes
-------
Update the patch
Bugs: DRILL-2639
https://issues.apache.org/jira/browse/DRILL-2639
Repository: drill-git
Description
-------
At planning for Union-All, block only the cases where implicit casting cannot resolve an output type
Diffs (updated)
-----
common/src/main/java/org/apache/drill/common/types/Types.java 015ee90
exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java aba6022
exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillUnionRelBase.java 932aa76
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java 796f0f7
exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 7033013
Diff: https://reviews.apache.org/r/32886/diff/
Testing
-------
QA, unit
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 32886: DRILL-2639: Planner bug -
RelOptPlanner.CannotPlanExceptio
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32886/
-----------------------------------------------------------
(Updated April 10, 2015, 12:52 a.m.)
Review request for drill and Aman Sinha.
Changes
-------
Methods consolidation
Bugs: DRILL-2639
https://issues.apache.org/jira/browse/DRILL-2639
Repository: drill-git
Description
-------
At planning for Union-All, block only the cases where implicit casting cannot resolve an output type
Diffs (updated)
-----
common/src/main/java/org/apache/drill/common/types/Types.java 015ee90
exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java aba6022
exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillUnionRelBase.java 932aa76
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java 796f0f7
exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 7033013
Diff: https://reviews.apache.org/r/32886/diff/
Testing
-------
QA, unit
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 32886: DRILL-2639: Planner bug -
RelOptPlanner.CannotPlanExceptio
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On April 9, 2015, 6:38 a.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java, line 506
> > <https://reviews.apache.org/r/32886/diff/1/?file=916507#file916507line506>
> >
> > I don't think you need a new utility method.. you can get the major type from Types.getMajorTypeFromName() and supply it the sqlType.getName() and you can get the minor type from major type.
Two reasons:
1. Types.getMajorTypeFromName() doese not cover all SqlTypeName. For example, SqlTypeName.SYMBOL would not be caught in any "case" in the switch logic. In my new method, I am sure every case is covered.
2. Using "String" type as switch logic could sometimes be risky. Especially here, when we can have the option to use 'enum', it seems easier and safer to use enum, as opposed to casting to String and switch-case.
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32886/#review79485
-----------------------------------------------------------
On April 6, 2015, 4:45 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32886/
> -----------------------------------------------------------
>
> (Updated April 6, 2015, 4:45 p.m.)
>
>
> Review request for drill and Aman Sinha.
>
>
> Bugs: DRILL-2639
> https://issues.apache.org/jira/browse/DRILL-2639
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> At planning for Union-All, block only the cases where implicit casting cannot resolve an output type
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java aba6022
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillUnionRelBase.java 932aa76
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java 796f0f7
> exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java fee1d6a
>
> Diff: https://reviews.apache.org/r/32886/diff/
>
>
> Testing
> -------
>
> QA, unit
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 32886: DRILL-2639: Planner bug -
RelOptPlanner.CannotPlanExceptio
Posted by Aman Sinha <as...@maprtech.com>.
> On April 9, 2015, 6:38 a.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java, line 506
> > <https://reviews.apache.org/r/32886/diff/1/?file=916507#file916507line506>
> >
> > I don't think you need a new utility method.. you can get the major type from Types.getMajorTypeFromName() and supply it the sqlType.getName() and you can get the minor type from major type.
>
> Sean Hsuan-Yi Chu wrote:
> Two reasons:
> 1. Types.getMajorTypeFromName() doese not cover all SqlTypeName. For example, SqlTypeName.SYMBOL would not be caught in any "case" in the switch logic. In my new method, I am sure every case is covered.
> 2. Using "String" type as switch logic could sometimes be risky. Especially here, when we can have the option to use 'enum', it seems easier and safer to use enum, as opposed to casting to String and switch-case.
We should not have 2 places to get essentially the same information. It would be best to reconcile your changes with the existing method.
- Aman
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32886/#review79485
-----------------------------------------------------------
On April 6, 2015, 4:45 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32886/
> -----------------------------------------------------------
>
> (Updated April 6, 2015, 4:45 p.m.)
>
>
> Review request for drill and Aman Sinha.
>
>
> Bugs: DRILL-2639
> https://issues.apache.org/jira/browse/DRILL-2639
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> At planning for Union-All, block only the cases where implicit casting cannot resolve an output type
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java aba6022
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillUnionRelBase.java 932aa76
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java 796f0f7
> exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java fee1d6a
>
> Diff: https://reviews.apache.org/r/32886/diff/
>
>
> Testing
> -------
>
> QA, unit
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 32886: DRILL-2639: Planner bug -
RelOptPlanner.CannotPlanExceptio
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On April 9, 2015, 6:38 a.m., Aman Sinha wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java, line 506
> > <https://reviews.apache.org/r/32886/diff/1/?file=916507#file916507line506>
> >
> > I don't think you need a new utility method.. you can get the major type from Types.getMajorTypeFromName() and supply it the sqlType.getName() and you can get the minor type from major type.
>
> Sean Hsuan-Yi Chu wrote:
> Two reasons:
> 1. Types.getMajorTypeFromName() doese not cover all SqlTypeName. For example, SqlTypeName.SYMBOL would not be caught in any "case" in the switch logic. In my new method, I am sure every case is covered.
> 2. Using "String" type as switch logic could sometimes be risky. Especially here, when we can have the option to use 'enum', it seems easier and safer to use enum, as opposed to casting to String and switch-case.
>
> Aman Sinha wrote:
> We should not have 2 places to get essentially the same information. It would be best to reconcile your changes with the existing method.
Agree. The two pieces were consolidated in the new patch.
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32886/#review79485
-----------------------------------------------------------
On April 10, 2015, 12:52 a.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32886/
> -----------------------------------------------------------
>
> (Updated April 10, 2015, 12:52 a.m.)
>
>
> Review request for drill and Aman Sinha.
>
>
> Bugs: DRILL-2639
> https://issues.apache.org/jira/browse/DRILL-2639
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> At planning for Union-All, block only the cases where implicit casting cannot resolve an output type
>
>
> Diffs
> -----
>
> common/src/main/java/org/apache/drill/common/types/Types.java 015ee90
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java aba6022
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillUnionRelBase.java 932aa76
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java 796f0f7
> exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java 7033013
>
> Diff: https://reviews.apache.org/r/32886/diff/
>
>
> Testing
> -------
>
> QA, unit
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 32886: DRILL-2639: Planner bug -
RelOptPlanner.CannotPlanExceptio
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32886/#review79485
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java
<https://reviews.apache.org/r/32886/#comment128894>
I don't think you need a new utility method.. you can get the major type from Types.getMajorTypeFromName() and supply it the sqlType.getName() and you can get the minor type from major type.
- Aman Sinha
On April 6, 2015, 4:45 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32886/
> -----------------------------------------------------------
>
> (Updated April 6, 2015, 4:45 p.m.)
>
>
> Review request for drill and Aman Sinha.
>
>
> Bugs: DRILL-2639
> https://issues.apache.org/jira/browse/DRILL-2639
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> At planning for Union-All, block only the cases where implicit casting cannot resolve an output type
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillRelOptUtil.java aba6022
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/common/DrillUnionRelBase.java 932aa76
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillOptiq.java 796f0f7
> exec/java-exec/src/test/java/org/apache/drill/TestUnionAll.java fee1d6a
>
> Diff: https://reviews.apache.org/r/32886/diff/
>
>
> Testing
> -------
>
> QA, unit
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>