DRILL-5936: Refactor MergingRecordBatch based on code inspection #1025
Conversation
|
@amansinha100 can you review this change? |
| boolean schemaChanged = false; | ||
|
|
||
| if (prevBatchWasFull) { | ||
| if (!prevBatchNotFull) { |
There was a problem hiding this comment.
The double negative makes it somewhat confusing. Perhaps rename the variable to 'prevBatchHasSpace' .
There was a problem hiding this comment.
@amansinha100 Addressed minor comment, please review delta and I'll squash it.
|
+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. |
vrozov
changed the title
| * @param node Reference to the next record to copy from the incoming batches | ||
| */ | ||
| private boolean copyRecordToOutgoingBatch(final Node node) { | ||
| assert outgoingPosition < OUTGOING_BATCH_SIZE |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
3 participants
No description provided.