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/07/06 22:31:16 UTC
Review Request 36219: DRILL-3292: SUM(constant) OVER(...) returns
wrong results
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36219/
-----------------------------------------------------------
Review request for drill, abdelhakim deneche and Aman Sinha.
Bugs: DRILL-3292
https://issues.apache.org/jira/browse/DRILL-3292
Repository: drill-git
Description
-------
At the stage of converting Prel to Pop (planning phase), ensure constants in Window are converted to LogicalExpression
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java 170438e
exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java 8676c28
Diff: https://reviews.apache.org/r/36219/diff/
Testing
-------
All required.
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 36219: DRILL-3292: SUM(constant) OVER(...) returns
wrong results
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36219/#review90799
-----------------------------------------------------------
Ship it!
Ship It!
- Aman Sinha
On July 7, 2015, 9:28 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36219/
> -----------------------------------------------------------
>
> (Updated July 7, 2015, 9:28 p.m.)
>
>
> Review request for drill, abdelhakim deneche and Aman Sinha.
>
>
> Bugs: DRILL-3292
> https://issues.apache.org/jira/browse/DRILL-3292
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> At the stage of converting Prel to Pop (planning phase), ensure constants in Window are converted to LogicalExpression
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java 170438e
> exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java 8676c28
>
> Diff: https://reviews.apache.org/r/36219/diff/
>
>
> Testing
> -------
>
> unit, tpch, functional
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 36219: DRILL-3292: SUM(constant) OVER(...) returns
wrong results
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36219/
-----------------------------------------------------------
(Updated July 7, 2015, 9:28 p.m.)
Review request for drill, abdelhakim deneche and Aman Sinha.
Changes
-------
new test case
Bugs: DRILL-3292
https://issues.apache.org/jira/browse/DRILL-3292
Repository: drill-git
Description
-------
At the stage of converting Prel to Pop (planning phase), ensure constants in Window are converted to LogicalExpression
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java 170438e
exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java 8676c28
Diff: https://reviews.apache.org/r/36219/diff/
Testing
-------
unit, tpch, functional
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 36219: DRILL-3292: SUM(constant) OVER(...) returns
wrong results
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36219/#review90577
-----------------------------------------------------------
+1 (non binding)
- abdelhakim deneche
On July 6, 2015, 9:23 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36219/
> -----------------------------------------------------------
>
> (Updated July 6, 2015, 9:23 p.m.)
>
>
> Review request for drill, abdelhakim deneche and Aman Sinha.
>
>
> Bugs: DRILL-3292
> https://issues.apache.org/jira/browse/DRILL-3292
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> At the stage of converting Prel to Pop (planning phase), ensure constants in Window are converted to LogicalExpression
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java 170438e
> exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java 8676c28
>
> Diff: https://reviews.apache.org/r/36219/diff/
>
>
> Testing
> -------
>
> unit, tpch, functional
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 36219: DRILL-3292: SUM(constant) OVER(...) returns
wrong results
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On July 7, 2015, 1:43 p.m., Aman Sinha wrote:
> > exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java, line 408
> > <https://reviews.apache.org/r/36219/diff/2/?file=1000522#file1000522line408>
> >
> > Pls modify this test case to have both constants and non-constant window aggregates as well as a rank() function since the creation of the Drill logical expression is sensitive to the indexes.
You can see the new patch to see if that one is good.
BTW, the baseline is constructed by Postgres.
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36219/#review90700
-----------------------------------------------------------
On July 6, 2015, 9:23 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36219/
> -----------------------------------------------------------
>
> (Updated July 6, 2015, 9:23 p.m.)
>
>
> Review request for drill, abdelhakim deneche and Aman Sinha.
>
>
> Bugs: DRILL-3292
> https://issues.apache.org/jira/browse/DRILL-3292
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> At the stage of converting Prel to Pop (planning phase), ensure constants in Window are converted to LogicalExpression
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java 170438e
> exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java 8676c28
>
> Diff: https://reviews.apache.org/r/36219/diff/
>
>
> Testing
> -------
>
> unit, tpch, functional
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 36219: DRILL-3292: SUM(constant) OVER(...) returns
wrong results
Posted by Aman Sinha <as...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36219/#review90700
-----------------------------------------------------------
exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java (line 408)
<https://reviews.apache.org/r/36219/#comment143841>
Pls modify this test case to have both constants and non-constant window aggregates as well as a rank() function since the creation of the Drill logical expression is sensitive to the indexes.
- Aman Sinha
On July 6, 2015, 9:23 p.m., Sean Hsuan-Yi Chu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36219/
> -----------------------------------------------------------
>
> (Updated July 6, 2015, 9:23 p.m.)
>
>
> Review request for drill, abdelhakim deneche and Aman Sinha.
>
>
> Bugs: DRILL-3292
> https://issues.apache.org/jira/browse/DRILL-3292
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> At the stage of converting Prel to Pop (planning phase), ensure constants in Window are converted to LogicalExpression
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java 170438e
> exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java 8676c28
>
> Diff: https://reviews.apache.org/r/36219/diff/
>
>
> Testing
> -------
>
> unit, tpch, functional
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 36219: DRILL-3292: SUM(constant) OVER(...) returns
wrong results
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36219/
-----------------------------------------------------------
(Updated July 6, 2015, 9:23 p.m.)
Review request for drill, abdelhakim deneche and Aman Sinha.
Changes
-------
new patch
Bugs: DRILL-3292
https://issues.apache.org/jira/browse/DRILL-3292
Repository: drill-git
Description
-------
At the stage of converting Prel to Pop (planning phase), ensure constants in Window are converted to LogicalExpression
Diffs (updated)
-----
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java 170438e
exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java 8676c28
Diff: https://reviews.apache.org/r/36219/diff/
Testing
-------
unit, tpch, functional
Thanks,
Sean Hsuan-Yi Chu
Re: Review Request 36219: DRILL-3292: SUM(constant) OVER(...) returns
wrong results
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
> On July 6, 2015, 9:02 p.m., abdelhakim deneche wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java, line 127
> > <https://reviews.apache.org/r/36219/diff/1/?file=1000449#file1000449line127>
> >
> > when we reach the else-block it means i >= fn.size() which means indexInConstants >= 0
> >
> > You should remove (indexInConstants >= 0) from the else condition as it is always true
Make sense. Will address in the new patch
- Sean Hsuan-Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36219/#review90564
-----------------------------------------------------------
On July 6, 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/36219/
> -----------------------------------------------------------
>
> (Updated July 6, 2015, 8:33 p.m.)
>
>
> Review request for drill, abdelhakim deneche and Aman Sinha.
>
>
> Bugs: DRILL-3292
> https://issues.apache.org/jira/browse/DRILL-3292
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> At the stage of converting Prel to Pop (planning phase), ensure constants in Window are converted to LogicalExpression
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java 170438e
> exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java 8676c28
>
> Diff: https://reviews.apache.org/r/36219/diff/
>
>
> Testing
> -------
>
> unit, tpch, functional
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 36219: DRILL-3292: SUM(constant) OVER(...) returns
wrong results
Posted by abdelhakim deneche <ad...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36219/#review90564
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java (line 123)
<https://reviews.apache.org/r/36219/#comment143666>
when we reach the else-block it means i >= fn.size() which means indexInConstants >= 0
You should remove (indexInConstants >= 0) from the else condition as it is always true
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java (line 131)
<https://reviews.apache.org/r/36219/#comment143667>
should we remove this block ?
- abdelhakim deneche
On July 6, 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/36219/
> -----------------------------------------------------------
>
> (Updated July 6, 2015, 8:33 p.m.)
>
>
> Review request for drill, abdelhakim deneche and Aman Sinha.
>
>
> Bugs: DRILL-3292
> https://issues.apache.org/jira/browse/DRILL-3292
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> At the stage of converting Prel to Pop (planning phase), ensure constants in Window are converted to LogicalExpression
>
>
> Diffs
> -----
>
> exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java 170438e
> exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java 8676c28
>
> Diff: https://reviews.apache.org/r/36219/diff/
>
>
> Testing
> -------
>
> unit, tpch, functional
>
>
> Thanks,
>
> Sean Hsuan-Yi Chu
>
>
Re: Review Request 36219: DRILL-3292: SUM(constant) OVER(...) returns
wrong results
Posted by Sean Hsuan-Yi Chu <hs...@usc.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36219/
-----------------------------------------------------------
(Updated July 6, 2015, 8:33 p.m.)
Review request for drill, abdelhakim deneche and Aman Sinha.
Changes
-------
patch
Bugs: DRILL-3292
https://issues.apache.org/jira/browse/DRILL-3292
Repository: drill-git
Description
-------
At the stage of converting Prel to Pop (planning phase), ensure constants in Window are converted to LogicalExpression
Diffs
-----
exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/WindowPrel.java 170438e
exec/java-exec/src/test/java/org/apache/drill/exec/TestWindowFunctions.java 8676c28
Diff: https://reviews.apache.org/r/36219/diff/
Testing (updated)
-------
unit, tpch, functional
Thanks,
Sean Hsuan-Yi Chu