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 2015/10/27 19:42:16 UTC
[GitHub] drill pull request: Drill 3952
GitHub user adeneche opened a pull request:
https://github.com/apache/drill/pull/222
Drill 3952
Improve Window Functions performance when not all batches are required to process the current batch
This patch also improves how the window functions operator handles limits by avoiding processing anymore batches after the kill signal has been received
@amansinha100 can you please review ?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/adeneche/incubator-drill DRILL-3952.final.rebased
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/222.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 #222
----
commit 969706c6443227dd6c345a3160c29ddc5f177dae
Author: adeneche <ad...@gmail.com>
Date: 2015-10-22T21:08:10Z
DRILL-3952: Improve Window Functions performance when not all batches are required to process the current batch
commit 5ed54a963c30a8742a706804df11942812a0a976
Author: adeneche <ad...@gmail.com>
Date: 2015-10-25T03:18:46Z
WindowFrameRecordBatch doesn't process anymore batch after killIncoming() has been called
----
---
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 3952
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/222#discussion_r43668976
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/DefaultFrameTemplate.java ---
@@ -287,6 +318,36 @@ private int countPeers(final int start) {
}
/**
+ * Counts how many rows are peer with the first row of the current frame. This is called when we don't require all
+ * batches of current partition to be processed at once.<br>
+ * Assumes the end of the frame has indeed been found, because of this it doesn't use partition.remaining to check
+ * the end of current partition but rather isSamePartition()
+ * @param start first row of current frame
+ * @return number of peer rows
+ */
+ private int countPeersPartialPartition(final int start) {
+ // current frame always starts from first batch
+ final VectorAccessible first = getCurrent();
+ int length = 0;
+
+ // count all rows that are in the same frame of starting row
+ // keep increasing length until we find first non peer row we reach the very
+ // last batch
+ for (WindowDataBatch batch : batches) {
+ final int recordCount = batch.getRecordCount();
+
+ // for every remaining row in the partition, count it if it's a peer row
+ for (int row = (batch == first) ? start : 0; row < recordCount; row++, length++) {
--- End diff --
This may be a relic from early implementations. I should be able to remove it now.
I may also be able to get rid of `updatePartitionSize()` but this may require too much work for this patch and may do it in a separate patch
---
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 3952
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/222#discussion_r43789532
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/WindowFramer.java ---
@@ -38,19 +40,33 @@ void setup(final List<WindowDataBatch> batches, final VectorContainer container,
void doWork() throws DrillException;
/**
- * check if current batch can be processed:
- * <ol>
- * <li>we have at least 2 saved batches</li>
- * <li>last partition of current batch ended</li>
- * </ol>
- * @return true if current batch can be processed, false otherwise
- */
- boolean canDoWork();
-
- /**
* @return number rows processed in last batch
*/
int getOutputCount();
void cleanup();
+
+ /**
+ * compares two rows from different batches (can be the same), if they have the same value for the partition by
+ * expression
+ * @param b1Index index of first row
+ * @param b1 batch for first row
+ * @param b2Index index of second row
+ * @param b2 batch for second row
+ * @return true if the rows are in the same partition
+ */
+ boolean isSamePartition(@Named("b1Index") int b1Index, @Named("b1") VectorAccessible b1,
+ @Named("b2Index") int b2Index, @Named("b2") VectorAccessible b2);
+
+ /**
+ * compares two rows from different batches (can be the same), if they have the same value for the order by
+ * expression
+ * @param b1Index index of first row
+ * @param b1 batch for first row
+ * @param b2Index index of second row
+ * @param b2 batch for second row
+ * @return true if the rows are in the same partition
--- End diff --
change to 'rows in the same partition and are peers'
---
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 3952
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/222#discussion_r43586223
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/DefaultFrameTemplate.java ---
@@ -287,6 +318,36 @@ private int countPeers(final int start) {
}
/**
+ * Counts how many rows are peer with the first row of the current frame. This is called when we don't require all
+ * batches of current partition to be processed at once.<br>
+ * Assumes the end of the frame has indeed been found, because of this it doesn't use partition.remaining to check
+ * the end of current partition but rather isSamePartition()
+ * @param start first row of current frame
+ * @return number of peer rows
+ */
+ private int countPeersPartialPartition(final int start) {
--- End diff --
This should be a long since the number of peers could easily be greater than Integer.MAX_VALUE.
---
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 3952
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on the pull request:
https://github.com/apache/drill/pull/222#issuecomment-153504706
+1. Per previous comments, there is room for improvement in terms of code organization and reducing the number of passes which I understand will be covered by DRILL-3662.
---
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 3952
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/222#issuecomment-153795912
thanks @amansinha100 , I updated DRILL-3662 description with the new refactoring tasks
---
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 3952
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/222
---
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 3952
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/222#discussion_r43658789
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/DefaultFrameTemplate.java ---
@@ -287,6 +318,36 @@ private int countPeers(final int start) {
}
/**
+ * Counts how many rows are peer with the first row of the current frame. This is called when we don't require all
+ * batches of current partition to be processed at once.<br>
+ * Assumes the end of the frame has indeed been found, because of this it doesn't use partition.remaining to check
+ * the end of current partition but rather isSamePartition()
+ * @param start first row of current frame
+ * @return number of peer rows
+ */
+ private int countPeersPartialPartition(final int start) {
--- End diff --
will fix
---
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 3952
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on the pull request:
https://github.com/apache/drill/pull/222#issuecomment-152853864
Overall, there are 2 categories of the window functions: one that require all batches of the partition before they can start processing and second that can start work with partial partition as long as we have all peer rows of the current row. It seems to me that some refactoring is needed to have derived classes dedicated to the 2 categories, otherwise the logic in the common code is getting complicated. What do you think ? This need not necessarily be done as part of this patch...but we should discuss.
---
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 3952
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/222#discussion_r43586353
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/DefaultFrameTemplate.java ---
@@ -287,6 +318,36 @@ private int countPeers(final int start) {
}
/**
+ * Counts how many rows are peer with the first row of the current frame. This is called when we don't require all
+ * batches of current partition to be processed at once.<br>
+ * Assumes the end of the frame has indeed been found, because of this it doesn't use partition.remaining to check
+ * the end of current partition but rather isSamePartition()
+ * @param start first row of current frame
+ * @return number of peer rows
+ */
+ private int countPeersPartialPartition(final int start) {
+ // current frame always starts from first batch
+ final VectorAccessible first = getCurrent();
+ int length = 0;
+
+ // count all rows that are in the same frame of starting row
+ // keep increasing length until we find first non peer row we reach the very
+ // last batch
+ for (WindowDataBatch batch : batches) {
+ final int recordCount = batch.getRecordCount();
+
+ // for every remaining row in the partition, count it if it's a peer row
+ for (int row = (batch == first) ? start : 0; row < recordCount; row++, length++) {
--- End diff --
The performance could be pretty bad due to the sequential scan through the remaining rows. The data is sorted right ?
---
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 3952
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/222#discussion_r43658717
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/DefaultFrameTemplate.java ---
@@ -287,6 +318,36 @@ private int countPeers(final int start) {
}
/**
+ * Counts how many rows are peer with the first row of the current frame. This is called when we don't require all
+ * batches of current partition to be processed at once.<br>
+ * Assumes the end of the frame has indeed been found, because of this it doesn't use partition.remaining to check
+ * the end of current partition but rather isSamePartition()
+ * @param start first row of current frame
+ * @return number of peer rows
+ */
+ private int countPeersPartialPartition(final int start) {
+ // current frame always starts from first batch
+ final VectorAccessible first = getCurrent();
+ int length = 0;
+
+ // count all rows that are in the same frame of starting row
+ // keep increasing length until we find first non peer row we reach the very
+ // last batch
+ for (WindowDataBatch batch : batches) {
+ final int recordCount = batch.getRecordCount();
+
+ // for every remaining row in the partition, count it if it's a peer row
+ for (int row = (batch == first) ? start : 0; row < recordCount; row++, length++) {
+ if (!isSamePartition(start, first, row, batch) || !isPeer(start, first, row, batch)) {
--- End diff --
why not. We still need to have a separate `isSamePartition` though
---
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 3952
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/222#discussion_r43586273
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/DefaultFrameTemplate.java ---
@@ -287,6 +318,36 @@ private int countPeers(final int start) {
}
/**
+ * Counts how many rows are peer with the first row of the current frame. This is called when we don't require all
+ * batches of current partition to be processed at once.<br>
+ * Assumes the end of the frame has indeed been found, because of this it doesn't use partition.remaining to check
+ * the end of current partition but rather isSamePartition()
+ * @param start first row of current frame
+ * @return number of peer rows
+ */
+ private int countPeersPartialPartition(final int start) {
+ // current frame always starts from first batch
+ final VectorAccessible first = getCurrent();
+ int length = 0;
+
+ // count all rows that are in the same frame of starting row
+ // keep increasing length until we find first non peer row we reach the very
+ // last batch
+ for (WindowDataBatch batch : batches) {
+ final int recordCount = batch.getRecordCount();
+
+ // for every remaining row in the partition, count it if it's a peer row
+ for (int row = (batch == first) ? start : 0; row < recordCount; row++, length++) {
+ if (!isSamePartition(start, first, row, batch) || !isPeer(start, first, row, batch)) {
--- End diff --
I think isPeer() could also check whether two rows are in the same partition...because the notion of peer is only valid within a partition. This will simplify the generated code a bit.
---
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 3952
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/222#issuecomment-153131868
Window operator has become increasingly complex. I already opened [DRILL-3662](https://issues.apache.org/jira/browse/DRILL-3662) to refactor the code.
---
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.
---