You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by adeneche <gi...@git.apache.org> on 2016/01/27 17:54:52 UTC
[GitHub] drill pull request: Drill 4260: Adding support for "some" custom f...
GitHub user adeneche opened a pull request:
https://github.com/apache/drill/pull/340
Drill 4260: Adding support for "some" custom frames
Adding support for the following frames:
RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED PRECEDING
ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW
RANGE BETWEEN CURRENT ROW AND CURRENT ROW
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/adeneche/incubator-drill DRILL-4260
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/340.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #340
----
commit c5c2eda7997c5dfb9335b02a25272c0e6ddc48b1
Author: adeneche <ad...@gmail.com>
Date: 2016-01-19T21:33:22Z
passing WindowPOP to window functions and framers
commit 82395aefce54eb89eb98fd7420f4fc6a4b369be1
Author: adeneche <ad...@gmail.com>
Date: 2016-01-19T21:34:59Z
WindowPOP.Bound to describe FRAME bounds
commit 9a73558317de28851e077b9406e41ee248fd9cfd
Author: adeneche <ad...@gmail.com>
Date: 2016-01-20T19:41:26Z
DRILL-4261: Add support for RANGE BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING
commit 4be83782143a5ad3e46b91a20acb5cc16a65a4a7
Author: adeneche <ad...@gmail.com>
Date: 2016-01-25T19:53:00Z
FrameSupportTemplate doesn't need to use Partition class
commit a1e484d42e847e37e3612cf874f4985ce98c1388
Author: adeneche <ad...@gmail.com>
Date: 2016-01-25T20:18:06Z
pass "isRows" to WindowPOP
commit 128572e3b3ea1feaf01b42935835899c2d890ad5
Author: adeneche <ad...@gmail.com>
Date: 2016-01-25T20:19:22Z
DRILL-4262: add support for ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW
commit 100477a84226f207d8979c0fec42770c49632c4b
Author: adeneche <ad...@gmail.com>
Date: 2016-01-26T21:42:52Z
DRILL-4263: add support for RANGE BETWEEN CURRENT ROW AND CURRENT ROW
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4260: Adding support for "some" custom f...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/340
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4260: Adding support for "some" custom f...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on the pull request:
https://github.com/apache/drill/pull/340#issuecomment-183070887
+1. I gave a couple of comments; overall LGTM.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4260: Adding support for "some" custom f...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/340#discussion_r53351144
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/window/TestWindowFrame.java ---
@@ -106,6 +106,60 @@ public void testMultipleFramers() throws Exception {
);
}
+ @Test
+ public void testUnboundedFollowing() throws Exception {
+ testBuilder()
+ .sqlQuery(getFile("window/q3.sql"), TEST_RES_PATH)
+ .ordered()
+ .sqlBaselineQuery(getFile("window/q4.sql"), TEST_RES_PATH)
+ .build()
+ .run();
+ }
+
+ @Test
+ public void testAggregateRowsUnboundedAndCurrentRow() throws Exception {
+ final String table = "dfs_test.`"+TEST_RES_PATH+"/window/b4.p4`";
+ testBuilder()
+ .sqlQuery(getFile("window/aggregate_rows_unbounded_current.sql"), table)
+ .ordered()
+ .sqlBaselineQuery(getFile("window/aggregate_rows_unbounded_current_baseline.sql"), table)
+ .build()
+ .run();
+ }
+
+ @Test
+ public void testLastValueRowsUnboundedAndCurrentRow() throws Exception {
+ final String table = "dfs_test.`"+TEST_RES_PATH+"/window/b4.p4`";
+ testBuilder()
+ .sqlQuery(getFile("window/last_value_rows_unbounded_current.sql"), table)
+ .unOrdered()
+ .sqlBaselineQuery(getFile("window/last_value_rows_unbounded_current_baseline.sql"), table)
+ .build()
+ .run();
+ }
+
+ @Test
+ public void testAggregateRangeCurrentAndCurrent() throws Exception {
--- End diff --
the added tests use the window/b4.p4 dataset that contains 4 partitions. The second partition has a frame that has 3 rows at the end of the 1st batch and 2 rows at the beginning of the second batch.
Is this enough, or should I generate another dataset with a frame that spans over more than 2 batches ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4260: Adding support for "some" custom f...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/340#discussion_r52186222
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/WindowPOP.java ---
@@ -33,26 +35,29 @@
private final NamedExpression[] withins;
private final NamedExpression[] aggregations;
private final Order.Ordering[] orderings;
- private final long start;
- private final long end;
+ private final boolean rows;
--- End diff --
The 'rows' variable typically could be misinterpreted to mean number of rows (here you are using it as ROWS vs RANGE)..maybe rename it ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4260: Adding support for "some" custom f...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/340#discussion_r52672160
--- Diff: exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/window/TestWindowFrame.java ---
@@ -106,6 +106,60 @@ public void testMultipleFramers() throws Exception {
);
}
+ @Test
+ public void testUnboundedFollowing() throws Exception {
+ testBuilder()
+ .sqlQuery(getFile("window/q3.sql"), TEST_RES_PATH)
+ .ordered()
+ .sqlBaselineQuery(getFile("window/q4.sql"), TEST_RES_PATH)
+ .build()
+ .run();
+ }
+
+ @Test
+ public void testAggregateRowsUnboundedAndCurrentRow() throws Exception {
+ final String table = "dfs_test.`"+TEST_RES_PATH+"/window/b4.p4`";
+ testBuilder()
+ .sqlQuery(getFile("window/aggregate_rows_unbounded_current.sql"), table)
+ .ordered()
+ .sqlBaselineQuery(getFile("window/aggregate_rows_unbounded_current_baseline.sql"), table)
+ .build()
+ .run();
+ }
+
+ @Test
+ public void testLastValueRowsUnboundedAndCurrentRow() throws Exception {
+ final String table = "dfs_test.`"+TEST_RES_PATH+"/window/b4.p4`";
+ testBuilder()
+ .sqlQuery(getFile("window/last_value_rows_unbounded_current.sql"), table)
+ .unOrdered()
+ .sqlBaselineQuery(getFile("window/last_value_rows_unbounded_current_baseline.sql"), table)
+ .build()
+ .run();
+ }
+
+ @Test
+ public void testAggregateRangeCurrentAndCurrent() throws Exception {
--- End diff --
For the RANGE frame specification, are there unit tests that have the same ORDER-BY value repeating across multiple batches ? This will form the peer group for the range...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4260: Adding support for "some" custom f...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/340#discussion_r53347747
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/FrameSupportTemplate.java ---
@@ -134,44 +142,67 @@ private void cleanPartition() {
* @throws DrillException if it can't write into the container
*/
private int processPartition(final int currentRow) throws DrillException {
- logger.trace("process partition {}, currentRow: {}, outputCount: {}", partition, currentRow, outputCount);
+ logger.trace("{} rows remaining to process, currentRow: {}, outputCount: {}", remainingRows, currentRow, outputCount);
setupWriteFirstValue(internal, container);
- int row = currentRow;
+ if (popConfig.isRows()) {
+ return processROWS(currentRow);
+ } else {
+ return processRANGE(currentRow);
+ }
+ }
+
+ private int processROWS(int row) throws DrillException {
+ //TODO we only need to call these once per batch
--- End diff --
We do the setup at the beginning of every partition. In case we have multiple partitions in the same batch setup should only be done once. To make the matters more complicated, if we are aggregating a single partition that spans multiple batches, we also need to do the setup for every batch.
The TODO is still valid. I create [DRILL-4413](https://issues.apache.org/jira/browse/DRILL-4413) to keep track of it
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4260: Adding support for "some" custom f...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/340#discussion_r53349474
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/config/WindowPOP.java ---
@@ -33,26 +35,29 @@
private final NamedExpression[] withins;
private final NamedExpression[] aggregations;
private final Order.Ordering[] orderings;
- private final long start;
- private final long end;
+ private final boolean rows;
--- End diff --
the SQL reference refers to this as "window frame units". Renaming to frameUnitsRows
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: Drill 4260: Adding support for "some" custom f...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/340#discussion_r52185980
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/FrameSupportTemplate.java ---
@@ -134,44 +142,67 @@ private void cleanPartition() {
* @throws DrillException if it can't write into the container
*/
private int processPartition(final int currentRow) throws DrillException {
- logger.trace("process partition {}, currentRow: {}, outputCount: {}", partition, currentRow, outputCount);
+ logger.trace("{} rows remaining to process, currentRow: {}, outputCount: {}", remainingRows, currentRow, outputCount);
setupWriteFirstValue(internal, container);
- int row = currentRow;
+ if (popConfig.isRows()) {
+ return processROWS(currentRow);
+ } else {
+ return processRANGE(currentRow);
+ }
+ }
+
+ private int processROWS(int row) throws DrillException {
+ //TODO we only need to call these once per batch
--- End diff --
Is this TODO still valid ? Is the setup being done per partition rather than per batch ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---