You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by vrozov <gi...@git.apache.org> on 2017/11/07 03:21:22 UTC
[GitHub] drill pull request #1025: DRILL-5936: Refactor MergingRecordBatch based on c...
GitHub user vrozov opened a pull request:
https://github.com/apache/drill/pull/1025
DRILL-5936: Refactor MergingRecordBatch based on code review
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vrozov/drill DRILL-5936
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1025.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 #1025
----
commit 82c04413d63733a8aad58e4b328af97e8f2ad6d5
Author: Vlad Rozov <vr...@apache.org>
Date: 2017-11-07T01:55:56Z
DRILL-5936: Refactor MergingRecordBatch based on code review
----
---
[GitHub] drill pull request #1025: DRILL-5936: Refactor MergingRecordBatch based on c...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/1025#discussion_r150335985
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java ---
@@ -795,6 +788,8 @@ private void generateComparisons(final ClassGenerator<?> g, final VectorAccessib
* @param node Reference to the next record to copy from the incoming batches
*/
private boolean copyRecordToOutgoingBatch(final Node node) {
+ assert outgoingPosition < OUTGOING_BATCH_SIZE
--- End diff --
Ok. Updated changes lgtm.
---
[GitHub] drill pull request #1025: DRILL-5936: Refactor MergingRecordBatch based on c...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/1025#discussion_r150329246
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java ---
@@ -795,6 +788,8 @@ private void generateComparisons(final ClassGenerator<?> g, final VectorAccessib
* @param node Reference to the next record to copy from the incoming batches
*/
private boolean copyRecordToOutgoingBatch(final Node node) {
+ assert outgoingPosition < OUTGOING_BATCH_SIZE
--- End diff --
Doing this assert check on each record would add some overhead. Since the caller already is checking outgoingBatchHasSpace in the while loop, this check is not needed. You could add a javadoc comment that caller is expected to do the validity check.
---
[GitHub] drill pull request #1025: DRILL-5936: Refactor MergingRecordBatch based on c...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1025#discussion_r150331389
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java ---
@@ -795,6 +788,8 @@ private void generateComparisons(final ClassGenerator<?> g, final VectorAccessib
* @param node Reference to the next record to copy from the incoming batches
*/
private boolean copyRecordToOutgoingBatch(final Node node) {
+ assert outgoingPosition < OUTGOING_BATCH_SIZE
--- End diff --
I added the assert to avoid possible errors during further code refactoring. As it is an assert that will not affect performance in production and there is another assert already, I'd prefer to keep it.
---
[GitHub] drill pull request #1025: DRILL-5936: Refactor MergingRecordBatch based on c...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1025
---
[GitHub] drill pull request #1025: DRILL-5936: Refactor MergingRecordBatch based on c...
Posted by vrozov <gi...@git.apache.org>.
Github user vrozov commented on a diff in the pull request:
https://github.com/apache/drill/pull/1025#discussion_r150326663
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java ---
@@ -177,11 +177,11 @@ public IterOutcome innerNext() {
}
boolean schemaChanged = false;
- if (prevBatchWasFull) {
+ if (!prevBatchNotFull) {
--- End diff --
@amansinha100 Addressed minor comment, please review delta and I'll squash it.
---
[GitHub] drill issue #1025: DRILL-5936: Refactor MergingRecordBatch based on code rev...
Posted by priteshm <gi...@git.apache.org>.
Github user priteshm commented on the issue:
https://github.com/apache/drill/pull/1025
@amansinha100 can you review this change?
---
[GitHub] drill issue #1025: DRILL-5936: Refactor MergingRecordBatch based on code rev...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on the issue:
https://github.com/apache/drill/pull/1025
+1 with a minor comment. In the commit message and JIRA it would be better to say 'code inspection' instead of code review which may be interpreted to mean the normal code review process.
---
[GitHub] drill pull request #1025: DRILL-5936: Refactor MergingRecordBatch based on c...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/1025#discussion_r150310556
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/mergereceiver/MergingRecordBatch.java ---
@@ -177,11 +177,11 @@ public IterOutcome innerNext() {
}
boolean schemaChanged = false;
- if (prevBatchWasFull) {
+ if (!prevBatchNotFull) {
--- End diff --
The double negative makes it somewhat confusing. Perhaps rename the variable to 'prevBatchHasSpace' .
---